GRO with non napi driver: BUG in __napi_complete

DISCUSSION Applied Frank Blaschka 5 participants 6 messages Mar 17, 2009 – Mar 27, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Frank Blaschka

author

2009-03-17 08:35 UTC #0 lore raw

Hi,

I try to activate GRO on a non napi driver (2.6.29-rc8). Running an iperf test causes a bug in __napi_complete.

kernel BUG at net/core/dev.c:2625!
illegal operation: 0001 <DD>#1<A8> PREEMPT SMP

Modules linked in:
CPU: 1 Not tainted 2.6.29-rc8-00124-g5bee17f #8
Process swapper (pid: 0, task: 000000002ff7ccc0, ksp: 000000002ff97d48)
Krnl PSW : 0404d00180000000 00000000002d1f4e (__napi_complete+0x82/0x88)
           R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 EA:3
...
<DD>  261.377396<A8> Call Trace:
<DD>  261.377400<A8> (<DD><00000000002cb752><A8> process_backlog+0xba/0x104)
<DD>  261.377410<A8>  <DD><00000000002cb5ba><A8> net_rx_action+0x102/0x1e0
<DD>  261.377418<A8>  <DD><000000000004921e><A8> __do_softirq+0x92/0x168
<DD>  261.377428<A8>  <DD><0000000000020936><A8> do_softirq+0x96/0xb0
<DD>  261.377436<A8>  <DD><00000000000493c0><A8> irq_exit+0x70/0x80
<DD>  261.377444<A8>  <DD><000000000025789c><A8> do_IRQ+0x174/0x194
<DD>  261.377455<A8>  <DD><00000000000258da><A8> io_return+0x0/0x8
<DD>  261.377464<A8>  <DD><00000000000246fe><A8> vtime_stop_cpu+0xb2/0xc0
<DD>  261.377473<A8> (<DD><00000007005b1007><A8> 0x7005b1007)

What is the intention process_backlog calls __napi_complete() instead of napi_complete(), this looks suspicious to me. Can anybody help?

Thanks,

Frank

Herbert Xu

2009-03-17 10:22 UTC #1 lore raw

Quoted 3 lines · Frank Blaschka

What is the intention process_backlog calls __napi_complete() instead of napi_complete(), this looks suspicious to me. Can anybody help?

You're absolutely right. Dave, we need this fix for both net and net-next.

gro: Fix legacy path napi_complete crash

On the legacy netif_rx path, I incorrectly tried to optimise the napi_complete call by using __napi_complete before we reenable IRQs. This simply doesn't work since we need to flush the held GRO packets first.

This patch fixes it by doing the obvious thing of reenabling IRQs first and then calling napi_complete.

Reported-by Frank Blaschka <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

net/core/dev.c +3 -2

index f112970..2565f6d 100644
@@ -2588,9 +2588,9 @@ static int process_backlog(struct napi_struct *napi, int quota)
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
 		if (!skb) {
-			__napi_complete(napi);
 			local_irq_enable();
-			break;
+			napi_complete(napi);
+			goto out;
 		}
 		local_irq_enable();

@@ -2599,6 +2599,7 @@ static int process_backlog(struct napi_struct *napi, int quota)
 
 	napi_gro_flush(napi);
 
+out:
 	return work;
 }

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer Applied

2009-03-17 20:11 UTC #2 lore raw

From: Herbert Xu <[email protected]>
Date: Tue, 17 Mar 2009 18:22:44 +0800

Quoted 15 lines · Frank Blaschka

Quoted 3 lines
What is the intention process_backlog calls __napi_complete() instead of napi_complete(), this looks suspicious to me. Can anybody help?
You're absolutely right. Dave, we need this fix for both net and net-next.
gro: Fix legacy path napi_complete crash
On the legacy netif_rx path, I incorrectly tried to optimise the napi_complete call by using __napi_complete before we reenable IRQs. This simply doesn't work since we need to flush the held GRO packets first.

Quoted 5 lines

This patch fixes it by doing the obvious thing of reenabling IRQs first and then calling napi_complete.
Reported-by Frank Blaschka <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied, thanks.

Tom Herbert

2009-03-27 19:05 UTC #3 lore raw

In reply to Herbert Xu

Quoted 8 lines

On the legacy netif_rx path, I incorrectly tried to optimise
the napi_complete call by using __napi_complete before we reenable IRQs. This simply doesn't work since we need to flush the held GRO packets first.
This patch fixes it by doing the obvious thing of reenabling IRQs first and then calling napi_complete.

Does this fix generate a race condition for a non-NAPI device? If netif_rx runs immediately after local_irq_enable it would queue a packet on the backlog queue and try to schedule napi (the latter has no effect because napi has not completed). On return from interrupt, napi_complete is done leaving a packet in the input queue but napi is not scheduled to process it.

Thanks,
Tom

Quoted 38 lines

Reported-by Frank Blaschka <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

net/core/dev.c +3 -2

index f112970..2565f6d 100644
@@ -2588,9 +2588,9 @@ static int process_backlog(struct napi_struct *napi, int quota)
                local_irq_disable();
                skb = __skb_dequeue(&queue->input_pkt_queue);
                if (!skb) {
-                       __napi_complete(napi);
                        local_irq_enable();
-                       break;
+                       napi_complete(napi);
+                       goto out;
                }
                local_irq_enable();

@@ -2599,6 +2599,7 @@ static int process_backlog(struct napi_struct *napi, int quota)
 
        napi_gro_flush(napi);
 
+out:
        return work;
  }

Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2009-03-27 22:50 UTC #4 lore raw

From: Tom Herbert <[email protected]>
Date: Fri, 27 Mar 2009 12:05:30 -0700

Quoted 9 lines

Quoted 8 lines
On the legacy netif_rx path, I incorrectly tried to optimise
the napi_complete call by using __napi_complete before we reenable IRQs. This simply doesn't work since we need to flush the held GRO packets first.
This patch fixes it by doing the obvious thing of reenabling IRQs first and then calling napi_complete.

Quoted 6 lines

Does this fix generate a race condition for a non-NAPI device? If netif_rx runs immediately after local_irq_enable it would queue a packet on the backlog queue and try to schedule napi (the latter has no effect because napi has not completed). On return from interrupt, napi_complete is done leaving a packet in the input queue but napi is not scheduled to process it.

Yes we know this version of Herbert's patch has that problem, read the rest of the thread and subsequent versions of the fix.

Jean-Pascal Billaud

2009-03-17 15:49 UTC #5 lore raw

In reply to Frank Blaschka

More generally, is there still a point to support non-napi drivers? Shouldn't we have a unique way to process traffic at the driver level?

--jp

Quoted 50 lines

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of Frank Blaschka
Sent: Tuesday, March 17, 2009 1:35 AM
To: [email protected]
Cc: David Miller; Herbert Xu
Subject: GRO with non napi driver: BUG in __napi_complete
Hi,
I try to activate GRO on a non napi driver (2.6.29-rc8). Running an iperf test
causes a bug in __napi_complete.
kernel BUG at net/core/dev.c:2625!
illegal operation: 0001 <DD>#1<A8> PREEMPT SMP
Modules linked in:
CPU: 1 Not tainted 2.6.29-rc8-00124-g5bee17f #8
Process swapper (pid: 0, task: 000000002ff7ccc0, ksp: 000000002ff97d48)
Krnl PSW : 0404d00180000000 00000000002d1f4e
(__napi_complete+0x82/0x88)
           R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 EA:3
...
<DD> 261.377396<A8> Call Trace:
<DD> 261.377400<A8> (<DD><00000000002cb752><A8>
process_backlog+0xba/0x104)
<DD> 261.377410<A8> <DD><00000000002cb5ba><A8>
net_rx_action+0x102/0x1e0
<DD> 261.377418<A8> <DD><000000000004921e><A8>
__do_softirq+0x92/0x168
<DD>  261.377428<A8>  <DD><0000000000020936><A8> do_softirq+0x96/0xb0
<DD>  261.377436<A8>  <DD><00000000000493c0><A8> irq_exit+0x70/0x80
<DD>  261.377444<A8>  <DD><000000000025789c><A8> do_IRQ+0x174/0x194
<DD>  261.377455<A8>  <DD><00000000000258da><A8> io_return+0x0/0x8
<DD> 261.377464<A8> <DD><00000000000246fe><A8> vtime_stop_cpu+0xb2/0xc0
<DD> 261.377473<A8> (<DD><00000007005b1007><A8> 0x7005b1007)
What is the intention process_backlog calls __napi_complete() instead of
napi_complete(), this looks suspicious to me. Can anybody help?
Thanks,
Frank
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html