[PATCH v2 net] r8169: fix NAPI handling under high load

[PATCH v2 net] r8169: fix NAPI handling under high load

[Heiner Kallweit]

rtl_rx() and rtl_tx() are called only if the respective bits are set
in the interrupt status register. Under high load NAPI may not be
able to process all data (work_done == budget) and it will schedule
subsequent calls to the poll callback.
rtl_ack_events() however resets the bits in the interrupt status
register, therefore subsequent calls to rtl8169_poll() won't call
rtl_rx() and rtl_tx() - chip interrupts are still disabled.

Fix this by calling rtl_rx() and rtl_tx() independent of the bits
set in the interrupt status register. Both functions will detect
if there's nothing to do for them.

Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- added "Fixes" tag
---
 drivers/net/ethernet/realtek/r8169.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 8c4f49adc..7caf3b7e9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
 	struct rtl8169_private *tp = container_of(napi, struct rtl8169_private, napi);
 	struct net_device *dev = tp->dev;
 	u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
-	int work_done= 0;
+	int work_done;
 	u16 status;
 
 	status = rtl_get_events(tp);
 	rtl_ack_events(tp, status & ~tp->event_slow);
 
-	if (status & RTL_EVENT_NAPI_RX)
-		work_done = rtl_rx(dev, tp, (u32) budget);
+	work_done = rtl_rx(dev, tp, (u32) budget);
 
-	if (status & RTL_EVENT_NAPI_TX)
-		rtl_tx(dev, tp);
+	rtl_tx(dev, tp);
 
 	if (status & tp->event_slow) {
 		enable_mask &= ~tp->event_slow;
-- 
2.19.1

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[Eric Dumazet]

On 10/18/2018 10:56 AM, Heiner Kallweit wrote:
> rtl_rx() and rtl_tx() are called only if the respective bits are set
> in the interrupt status register. Under high load NAPI may not be
> able to process all data (work_done == budget) and it will schedule
> subsequent calls to the poll callback.
> rtl_ack_events() however resets the bits in the interrupt status
> register, therefore subsequent calls to rtl8169_poll() won't call
> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
> 
> Fix this by calling rtl_rx() and rtl_tx() independent of the bits
> set in the interrupt status register. Both functions will detect
> if there's nothing to do for them.
> 
> Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - added "Fixes" tag
> ---
>  drivers/net/ethernet/realtek/r8169.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 8c4f49adc..7caf3b7e9 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
>  	struct rtl8169_private *tp = container_of(napi, struct rtl8169_private, napi);
>  	struct net_device *dev = tp->dev;
>  	u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
> -	int work_done= 0;
> +	int work_done;
>  	u16 status;
>  
>  	status = rtl_get_events(tp);
>  	rtl_ack_events(tp, status & ~tp->event_slow);
>  
> -	if (status & RTL_EVENT_NAPI_RX)
> -		work_done = rtl_rx(dev, tp, (u32) budget);
> +	work_done = rtl_rx(dev, tp, (u32) budget);
>  
> -	if (status & RTL_EVENT_NAPI_TX)
> -		rtl_tx(dev, tp);
> +	rtl_tx(dev, tp);
>  
>  	if (status & tp->event_slow) {
>  		enable_mask &= ~tp->event_slow;
> 

One has to wonder why rtl8169_poll(), which might be called in a loop under DOS,
has to call rtl_ack_events() ?

Should not this be called one time from hard irq handler instead ?

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[Heiner Kallweit]

On 18.10.2018 20:02, Eric Dumazet wrote:
> 
> 
> On 10/18/2018 10:56 AM, Heiner Kallweit wrote:
>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>> in the interrupt status register. Under high load NAPI may not be
>> able to process all data (work_done == budget) and it will schedule
>> subsequent calls to the poll callback.
>> rtl_ack_events() however resets the bits in the interrupt status
>> register, therefore subsequent calls to rtl8169_poll() won't call
>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>
>> Fix this by calling rtl_rx() and rtl_tx() independent of the bits
>> set in the interrupt status register. Both functions will detect
>> if there's nothing to do for them.
>>
>> Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - added "Fixes" tag
>> ---
>>  drivers/net/ethernet/realtek/r8169.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 8c4f49adc..7caf3b7e9 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
>>  	struct rtl8169_private *tp = container_of(napi, struct rtl8169_private, napi);
>>  	struct net_device *dev = tp->dev;
>>  	u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
>> -	int work_done= 0;
>> +	int work_done;
>>  	u16 status;
>>  
>>  	status = rtl_get_events(tp);
>>  	rtl_ack_events(tp, status & ~tp->event_slow);
>>  
>> -	if (status & RTL_EVENT_NAPI_RX)
>> -		work_done = rtl_rx(dev, tp, (u32) budget);
>> +	work_done = rtl_rx(dev, tp, (u32) budget);
>>  
>> -	if (status & RTL_EVENT_NAPI_TX)
>> -		rtl_tx(dev, tp);
>> +	rtl_tx(dev, tp);
>>  
>>  	if (status & tp->event_slow) {
>>  		enable_mask &= ~tp->event_slow;
>>
> 
> One has to wonder why rtl8169_poll(), which might be called in a loop under DOS,
> has to call rtl_ack_events() ?
> 
> Should not this be called one time from hard irq handler instead ?
> 
Yes, it should. I have a patch in my tree including this, in addition it
moves everything from rtl_slow_event_work() to the hard irq handler.
However, due to recent changes, this patch may not apply cleanly to
older kernel versions.
Calling rtl_ack_events() in the poll callback isn't nice but not directly
a bug, so I'd prefer to submit this to net-next.

> 

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 18 Oct 2018 19:56:01 +0200

> rtl_rx() and rtl_tx() are called only if the respective bits are set
> in the interrupt status register. Under high load NAPI may not be
> able to process all data (work_done == budget) and it will schedule
> subsequent calls to the poll callback.
> rtl_ack_events() however resets the bits in the interrupt status
> register, therefore subsequent calls to rtl8169_poll() won't call
> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
> 
> Fix this by calling rtl_rx() and rtl_tx() independent of the bits
> set in the interrupt status register. Both functions will detect
> if there's nothing to do for them.
> 
> Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - added "Fixes" tag

Applied and queued up for -stable, thanks!

And I agree that moving the IRQ ACKing to the hw irq handler is
net-next material.

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[Francois Romieu]

Eric Dumazet <eric.dumazet@gmail.com> :
[...]
> One has to wonder why rtl8169_poll(), which might be called in a loop under DOS,
> has to call rtl_ack_events() ?

So as to cover a wider temporal range before any event can trigger an
extra irq. I was more worried about irq cost than about IO cost (and
I still am).

-- 
Ueimor

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[Eric Dumazet]

On 10/18/2018 03:59 PM, Francois Romieu wrote:
> Eric Dumazet <eric.dumazet@gmail.com> :
> [...]
>> One has to wonder why rtl8169_poll(), which might be called in a loop under DOS,
>> has to call rtl_ack_events() ?
> 
> So as to cover a wider temporal range before any event can trigger an
> extra irq. I was more worried about irq cost than about IO cost (and
> I still am).
> 
Normally the IRQ would not be enabled under DOS.

Only when a poll would receive less packets than the budget
the following would normally allow the device to send another IRQ

if (work_done < budget) {
    napi_complete_done(napi, work_done);
    rtl_irq_enable(tp, enable_mask);
    mmiowb();
}

Re: [PATCH v2 net] r8169: fix NAPI handling under high load

[Francois Romieu]

Eric Dumazet <eric.dumazet@gmail.com> :
> On 10/18/2018 03:59 PM, Francois Romieu wrote:
> > Eric Dumazet <eric.dumazet@gmail.com> :
> > [...]
> >> One has to wonder why rtl8169_poll(), which might be called in a loop under DOS,
> >> has to call rtl_ack_events() ?
> > 
> > So as to cover a wider temporal range before any event can trigger an
> > extra irq. I was more worried about irq cost than about IO cost (and
> > I still am).
> > 
> Normally the IRQ would not be enabled under DOS.

Yes.

My concern was not the DOS situation when NAPI runs at full speed.
As far as I was able to experiment with it, the driver did not seem
too bad here.

The location of the ack targets the interim situation where the IRQ
rate can increase before NAPI kicks in. By increasing the time range
whose events can be acked, the maximum irq rate should be lowered.

-- 
Ueimor