Re: r8169: transmit queue timeouts and IRQ masking

[Heiner Kallweit <hkallweit1@gmail.com>]

On 10.05.2024 00:24, Ken Milmore wrote:
> On 08/05/2024 22:14, Heiner Kallweit wrote:
>>
>> Re-reading &tp->napi may be racy, and I think the code delivers
>> a wrong result if NAPI_STATE_SCHEDand NAPI_STATE_DISABLE
>> both are set.
>>
>>>  out:
>>>         rtl_ack_events(tp, status);
>>
>> The following uses a modified version of napi_schedule_prep()
>> to avoid re-reading the napi state.
>> We would have to see whether this extension to the net core is
>> acceptable, as r8169 would be the only user for now.
>> For testing it's one patch, for submitting it would need to be
>> splitted.
>>
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c |  6 ++++--
>>  include/linux/netdevice.h                 |  7 ++++++-
>>  net/core/dev.c                            | 12 ++++++------
>>  3 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index eb329f0ab..94b97a16d 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -4639,6 +4639,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>>  {
>>  	struct rtl8169_private *tp = dev_instance;
>>  	u32 status = rtl_get_events(tp);
>> +	int ret;
>>  
>>  	if ((status & 0xffff) == 0xffff || !(status & tp->irq_mask))
>>  		return IRQ_NONE;
>> @@ -4657,10 +4658,11 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>>  		rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
>>  	}
>>  
>> -	if (napi_schedule_prep(&tp->napi)) {
>> +	ret = __napi_schedule_prep(&tp->napi);
>> +	if (ret >= 0)
>>  		rtl_irq_disable(tp);
>> +	if (ret > 0)
>>  		__napi_schedule(&tp->napi);
>> -	}
>>  out:
>>  	rtl_ack_events(tp, status);
>>  
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 42b9e6dc6..3df560264 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -498,7 +498,12 @@ static inline bool napi_is_scheduled(struct napi_struct *n)
>>  	return test_bit(NAPI_STATE_SCHED, &n->state);
>>  }
>>  
>> -bool napi_schedule_prep(struct napi_struct *n);
>> +int __napi_schedule_prep(struct napi_struct *n);
>> +
>> +static inline bool napi_schedule_prep(struct napi_struct *n)
>> +{
>> +	return __napi_schedule_prep(n) > 0;
>> +}
>>  
>>  /**
>>   *	napi_schedule - schedule NAPI poll
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 4bf081c5a..126eab121 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6102,21 +6102,21 @@ void __napi_schedule(struct napi_struct *n)
>>  EXPORT_SYMBOL(__napi_schedule);
>>  
>>  /**
>> - *	napi_schedule_prep - check if napi can be scheduled
>> + *	__napi_schedule_prep - check if napi can be scheduled
>>   *	@n: napi context
>>   *
>>   * Test if NAPI routine is already running, and if not mark
>>   * it as running.  This is used as a condition variable to
>> - * insure only one NAPI poll instance runs.  We also make
>> - * sure there is no pending NAPI disable.
>> + * insure only one NAPI poll instance runs. Return -1 if
>> + * there is a pending NAPI disable.
>>   */
>> -bool napi_schedule_prep(struct napi_struct *n)
>> +int __napi_schedule_prep(struct napi_struct *n)
>>  {
>>  	unsigned long new, val = READ_ONCE(n->state);
>>  
>>  	do {
>>  		if (unlikely(val & NAPIF_STATE_DISABLE))
>> -			return false;
>> +			return -1;
>>  		new = val | NAPIF_STATE_SCHED;
>>  
>>  		/* Sets STATE_MISSED bit if STATE_SCHED was already set
>> @@ -6131,7 +6131,7 @@ bool napi_schedule_prep(struct napi_struct *n)
>>  
>>  	return !(val & NAPIF_STATE_SCHED);
>>  }
>> -EXPORT_SYMBOL(napi_schedule_prep);
>> +EXPORT_SYMBOL(__napi_schedule_prep);
>>  
>>  /**
>>   * __napi_schedule_irqoff - schedule for receive
> 
> Here is a possible alternative (albeit expensive), using a flag in the driver:
> 
> diff --git linux-source-6.1~/drivers/net/ethernet/realtek/r8169_main.c linux-source-6.1/drivers/net/ethernet/realtek/r8169_main.c
> index 6e34177..d703af1 100644
> --- linux-source-6.1~/drivers/net/ethernet/realtek/r8169_main.c
> +++ linux-source-6.1/drivers/net/ethernet/realtek/r8169_main.c
> @@ -579,6 +579,7 @@ enum rtl_flag {
>         RTL_FLAG_TASK_RESET_PENDING,
>         RTL_FLAG_TASK_RESET_NO_QUEUE_WAKE,
>         RTL_FLAG_TASK_TX_TIMEOUT,
> +       RTL_FLAG_IRQ_DISABLED,
>         RTL_FLAG_MAX
>  };
>  
> @@ -4609,6 +4610,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  
>         if (napi_schedule_prep(&tp->napi)) {
>                 rtl_irq_disable(tp);
> +               set_bit(RTL_FLAG_IRQ_DISABLED, tp->wk.flags);
>                 __napi_schedule(&tp->napi);
>         }
>  out:
> @@ -4655,12 +4657,17 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
>         struct net_device *dev = tp->dev;
>         int work_done;
>  
> +       if (!test_and_set_bit(RTL_FLAG_IRQ_DISABLED, tp->wk.flags))
> +               rtl_irq_disable(tp);
> +
>         rtl_tx(dev, tp, budget);
>  
>         work_done = rtl_rx(dev, tp, budget);
>  
> -       if (work_done < budget && napi_complete_done(napi, work_done))
> +       if (work_done < budget && napi_complete_done(napi, work_done)) {
> +               clear_bit(RTL_FLAG_IRQ_DISABLED, tp->wk.flags);
>                 rtl_irq_enable(tp);
> +       }
>  
>         return work_done;
>  }
> 
> 
> 
> 

Nice idea. The following is a simplified version.
It's based on the thought that between scheduling NAPI and start of NAPI
polling interrupts don't hurt.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index e5ea827a2..7b04dfecc 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -592,6 +592,7 @@ enum rtl_flag {
 	RTL_FLAG_TASK_RESET_PENDING,
 	RTL_FLAG_TASK_RESET_NO_QUEUE_WAKE,
 	RTL_FLAG_TASK_TX_TIMEOUT,
+	RTL_FLAG_IRQ_DISABLED,
 	RTL_FLAG_MAX
 };
 
@@ -4657,10 +4658,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 		rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
 	}
 
-	if (napi_schedule_prep(&tp->napi)) {
-		rtl_irq_disable(tp);
-		__napi_schedule(&tp->napi);
-	}
+	napi_schedule(&tp->napi);
 out:
 	rtl_ack_events(tp, status);
 
@@ -4714,12 +4712,17 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
 	struct net_device *dev = tp->dev;
 	int work_done;
 
+	if (!test_and_set_bit(RTL_FLAG_IRQ_DISABLED, tp->wk.flags))
+		rtl_irq_disable(tp);
+
 	rtl_tx(dev, tp, budget);
 
 	work_done = rtl_rx(dev, tp, budget);
 
-	if (work_done < budget && napi_complete_done(napi, work_done))
+	if (work_done < budget && napi_complete_done(napi, work_done)) {
+		clear_bit(RTL_FLAG_IRQ_DISABLED, tp->wk.flags);
 		rtl_irq_enable(tp);
+	}
 
 	return work_done;
 }
-- 
2.45.0