Re: Revert "gro: Fix legacy path napi_complete crash",

Jarek Poplawski
2009-03-26T09:18:32Z

On Wed, Mar 25, 2009 at 08:20:50PM -0700, David Miller wrote:
...
> Adam Richter has successfully tested Jarek's variant, and if Ingo's
> tests show that it makes his problem go away too then I'm definitely
> going with Jarek's patch.

I hope this patch version on top of Herbert's revert could be useful
then.

Thanks,
Jarek P.
-------------------->
GRO: Re-enable GRO on legacy netif_rx path.

Replacing __napi_complete() with napi_complete() in process_backlog()
caused various breaks for non-napi NICs. It looks like the following
scenario can happen:

process_backlog() netif_rx()

if (!skb)
local_irq_enable()
if (queue.qlen) //NO
napi_schedule() //NOTHING
__skb_queue_tail() //qlen > 0
napi_complete()
... ...
Every next netif_rx() sees
qlen > 0, so napi is never
scheduled again.

This patch fixes it by open-coding napi_complete() with additional
check for empty queue before __napi_complete().

With help of debugging by Ingo Molnar, Sascha Hauer and Herbert Xu.

Reported-by: Ingo Molnar <mingo@elte.hu>
Tested-by: Adam Richter <adam_richter2004@yahoo.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---
(vs. 2.6.29 with Herbert's "GRO: Disable GRO on legacy netif_rx path")

diff -Nurp a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c 2009-03-26 08:32:51.000000000 +0000
+++ b/net/core/dev.c 2009-03-26 08:37:58.000000000 +0000
@@ -2588,15 +2588,22 @@ static int process_backlog(struct napi_s
local_irq_disable();
skb = __skb_dequeue(&queue->input_pkt_queue);
if (!skb) {
- __napi_complete(napi);
local_irq_enable();
- break;
+ napi_gro_flush(napi);
+ local_irq_disable();
+ if (skb_queue_empty(&queue->input_pkt_queue))
+ __napi_complete(napi);
+ local_irq_enable();
+ goto out;
}
local_irq_enable();

- netif_receive_skb(skb);
+ napi_gro_receive(napi, skb);
} while (++work < quota && jiffies == start_time);

+ napi_gro_flush(napi);
+
+out:
return work;
}