From: Thomas De Schampheleire <hidden> Date: 2011-07-12 09:23:32
Hi,
I'm adding the linuxppc-dev mailing list since this may be pointing to
an irq/softirq problem in the powerpc architecture-specific code...
On Tue, Jul 12, 2011 at 8:58 AM, Eric Dumazet [off-list ref] wrot=
e:
Le mardi 12 juillet 2011 =E0 08:38 +0200, Ronny Meeus a =E9crit :
quoted
On Tue, Jul 12, 2011 at 5:27 AM, David Miller [off-list ref] wrot=
I was running a test: 1 application was sending raw Ethernet packets
on a physical looped interface while a second application was
receiving packets, so the latter application receives each packet 2
times (once while sending from the context of the first application
and a second time while receiving from the hardware). =A0After some
time, the test blocks due to a spinlock reentrance issue in
af_packet. Both the sending application and the softIRQ receiving
packets enter the spinlock code. After applying the patch below, the
issue is resolved.
Signed-off-by: Ronny Meeus <redacted>
The packet receive hooks should always be called with software
interrupts disabled, it is a bug if this is not happening. =A0Your
patch should not be necessary at all.
Can you be a bit more specific on where the software interrupts should
be disabled?
Below you find the information I get after switching on the spin_lock
issue detection in the kernel.
In this run also I-PIPE was active but this issue is also seen with
I-PIPE disabled.
This seems a bug, but in softirq handling in your arch
Note that the reason we are seeing this problem, may be because the
kernel we are using contains some patches from Freescale.
Specifically, in dev_queue_xmit(), support is added for hardware queue
handling, just before entering the rcu_read_lock_bh():
if (dev->features & NETIF_F_HW_QDISC) {
txq =3D dev_pick_tx(dev, skb);
return dev_hard_start_xmit(skb, dev, txq);
}
/* Disable soft irqs for various locks below. Also
* stops preemption for RCU.
*/
rcu_read_lock_bh();
We just tried moving the escaping to dev_hard_start_xmit() after
taking the lock, but this gives a large number of other problems, e.g.
[ 78.662428] BUG: sleeping function called from invalid context at
mm/slab.c:3101
[ 78.751004] in_atomic(): 1, irqs_disabled(): 0, pid: 1908, name:
send_eth_socket
[ 78.839582] Call Trace:
[ 78.868784] [ec537b70] [c000789c] show_stack+0x78/0x18c (unreliable)
[ 78.944905] [ec537bb0] [c0022900] __might_sleep+0x100/0x118
[ 79.011636] [ec537bc0] [c00facc4] kmem_cache_alloc+0x48/0x118
[ 79.080446] [ec537be0] [c02cd0e8] __alloc_skb+0x50/0x130
[ 79.144047] [ec537c00] [c02cdf5c] skb_copy+0x44/0xc8
[ 79.203478] [ec537c20] [c029f904] dpa_tx+0x154/0x758
[ 79.262907] [ec537c80] [c02d78ec] dev_hard_start_xmit+0x424/0x588
[ 79.335878] [ec537cc0] [c02d7aac] dev_queue_xmit+0x5c/0x3a4
[ 79.402602] [ec537cf0] [c0338d4c] packet_sendmsg+0x8c4/0x988
[ 79.470363] [ec537d70] [c02c3838] sock_sendmsg+0x90/0xb4
[ 79.533960] [ec537e40] [c02c4420] sys_sendto+0xdc/0x120
[ 79.596514] [ec537f10] [c02c57d0] sys_socketcall+0x148/0x210
[ 79.664287] [ec537f40] [c001084c] ret_from_syscall+0x0/0x3c
[ 79.731015] --- Exception: c01 at 0x48051f00
[ 79.731019] LR =3D 0x4808e030
Note that this may just be the cause for us seeing this problem. If
indeed the main problem is irq_exit() invoking softirqs in a locked
context, then this patch adding hardware queue support is not really
relevant.
Any suggestions from the developers at linuxppc-dev are very welcome...
Thomas
From: David Miller <davem@davemloft.net> Date: 2011-07-12 10:02:21
From: Thomas De Schampheleire <redacted>
Date: Tue, 12 Jul 2011 11:23:28 +0200
Note that the reason we are seeing this problem, may be because the
kernel we are using contains some patches from Freescale.
Specifically, in dev_queue_xmit(), support is added for hardware queue
handling, just before entering the rcu_read_lock_bh():
if (dev->features & NETIF_F_HW_QDISC) {
txq = dev_pick_tx(dev, skb);
return dev_hard_start_xmit(skb, dev, txq);
}
/* Disable soft irqs for various locks below. Also
* stops preemption for RCU.
*/
rcu_read_lock_bh();
We just tried moving the escaping to dev_hard_start_xmit() after
taking the lock, but this gives a large number of other problems, e.g.
This is definitely why you are seeing this behavior.
You cannot invoke dev_hard_start_xmit() without softirqs
being disabled. It breaks everything.
This is what happens when you integrate networking patches
which were not reviewed and vetted on netdev.
From: Eric Dumazet <hidden> Date: 2011-07-12 10:10:26
Le mardi 12 juillet 2011 à 11:23 +0200, Thomas De Schampheleire a
écrit :
Hi,
I'm adding the linuxppc-dev mailing list since this may be pointing to
an irq/softirq problem in the powerpc architecture-specific code...
Note that the reason we are seeing this problem, may be because the
kernel we are using contains some patches from Freescale.
Specifically, in dev_queue_xmit(), support is added for hardware queue
handling, just before entering the rcu_read_lock_bh():
Oh well, what a mess.
if (dev->features & NETIF_F_HW_QDISC) {
txq = dev_pick_tx(dev, skb);
return dev_hard_start_xmit(skb, dev, txq);
This need to be :
local_bh_disable();
rc = dev_hard_start_xmit(skb, dev, txq);
local_bh_enable();
return rc;
}
/* Disable soft irqs for various locks below. Also
* stops preemption for RCU.
*/
rcu_read_lock_bh();
We just tried moving the escaping to dev_hard_start_xmit() after
taking the lock, but this gives a large number of other problems, e.g.
[ 78.662428] BUG: sleeping function called from invalid context at
mm/slab.c:3101
[ 78.751004] in_atomic(): 1, irqs_disabled(): 0, pid: 1908, name:
send_eth_socket
[ 78.839582] Call Trace:
[ 78.868784] [ec537b70] [c000789c] show_stack+0x78/0x18c (unreliable)
[ 78.944905] [ec537bb0] [c0022900] __might_sleep+0x100/0x118
[ 79.011636] [ec537bc0] [c00facc4] kmem_cache_alloc+0x48/0x118
[ 79.080446] [ec537be0] [c02cd0e8] __alloc_skb+0x50/0x130
[ 79.144047] [ec537c00] [c02cdf5c] skb_copy+0x44/0xc8
[ 79.203478] [ec537c20] [c029f904] dpa_tx+0x154/0x758
doing GFP_KERNEL allocations in dpa_tx() is wrong, for sure.
[ 79.262907] [ec537c80] [c02d78ec] dev_hard_start_xmit+0x424/0x588
[ 79.335878] [ec537cc0] [c02d7aac] dev_queue_xmit+0x5c/0x3a4
[ 79.402602] [ec537cf0] [c0338d4c] packet_sendmsg+0x8c4/0x988
[ 79.470363] [ec537d70] [c02c3838] sock_sendmsg+0x90/0xb4
[ 79.533960] [ec537e40] [c02c4420] sys_sendto+0xdc/0x120
[ 79.596514] [ec537f10] [c02c57d0] sys_socketcall+0x148/0x210
[ 79.664287] [ec537f40] [c001084c] ret_from_syscall+0x0/0x3c
[ 79.731015] --- Exception: c01 at 0x48051f00
[ 79.731019] LR = 0x4808e030
Note that this may just be the cause for us seeing this problem. If
indeed the main problem is irq_exit() invoking softirqs in a locked
context, then this patch adding hardware queue support is not really
relevant.
irq_exit() is fine. This is because BH are not masked because of the
Freescale patches.
Really, suggesting an af_packet patch to solve a problem introduced in
an out of tree patch is insane.
You guys hould have clearly stated you were using an alien kernel.
On Tue, Jul 12, 2011 at 12:10 PM, Eric Dumazet [off-list ref] wro=
te:
Le mardi 12 juillet 2011 =E0 11:23 +0200, Thomas De Schampheleire a
=E9crit :
quoted
Hi,
I'm adding the linuxppc-dev mailing list since this may be pointing to
an irq/softirq problem in the powerpc architecture-specific code...
quoted
Note that the reason we are seeing this problem, may be because the
kernel we are using contains some patches from Freescale.
Specifically, in dev_queue_xmit(), support is added for hardware queue
handling, just before entering the rcu_read_lock_bh():
=A0 =A0 =A0 =A0 }
=A0 =A0 =A0 =A0 /* Disable soft irqs for various locks below. Also
=A0 =A0 =A0 =A0 =A0* stops preemption for RCU.
=A0 =A0 =A0 =A0 =A0*/
=A0 =A0 =A0 =A0 rcu_read_lock_bh();
We just tried moving the escaping to dev_hard_start_xmit() after
taking the lock, but this gives a large number of other problems, e.g.
[ =A0 78.662428] BUG: sleeping function called from invalid context at
mm/slab.c:3101
[ =A0 78.751004] in_atomic(): 1, irqs_disabled(): 0, pid: 1908, name:
send_eth_socket
[ =A0 78.839582] Call Trace:
[ =A0 78.868784] [ec537b70] [c000789c] show_stack+0x78/0x18c (unreliable=
doing GFP_KERNEL allocations in dpa_tx() is wrong, for sure.
quoted
[ =A0 79.262907] [ec537c80] [c02d78ec] dev_hard_start_xmit+0x424/0x588
[ =A0 79.335878] [ec537cc0] [c02d7aac] dev_queue_xmit+0x5c/0x3a4
[ =A0 79.402602] [ec537cf0] [c0338d4c] packet_sendmsg+0x8c4/0x988
[ =A0 79.470363] [ec537d70] [c02c3838] sock_sendmsg+0x90/0xb4
[ =A0 79.533960] [ec537e40] [c02c4420] sys_sendto+0xdc/0x120
[ =A0 79.596514] [ec537f10] [c02c57d0] sys_socketcall+0x148/0x210
[ =A0 79.664287] [ec537f40] [c001084c] ret_from_syscall+0x0/0x3c
[ =A0 79.731015] --- Exception: c01 at 0x48051f00
[ =A0 79.731019] =A0 =A0 LR =3D 0x4808e030
Note that this may just be the cause for us seeing this problem. If
indeed the main problem is irq_exit() invoking softirqs in a locked
context, then this patch adding hardware queue support is not really
relevant.
irq_exit() is fine. This is because BH are not masked because of the
Freescale patches.
Really, suggesting an af_packet patch to solve a problem introduced in
an out of tree patch is insane.
You guys hould have clearly stated you were using an alien kernel.
Sorry for not mentioning we were using a patched kernel.
I was not aware that the code involved was patched by the FreeScale
patches we applied. The code found in the stack dumps is not
implemented in FSL specific files.
While reading the code of af_packet I saw that the spin_lock_bh is
used in several places while this is not the case in the tpacket_rcv
function. Since we had a locking issue in that code, I thought that my
patch would be OK.
I was not aware that for that specific function (tpacket_rcv) a
different lock primitive must be used. A suggestion for improvement:
it would be better to document this pre-condition in the code.
After doing the change you proposed our code now looks like:
---/* Disable soft irqs for various locks below. Also
--- * stops preemption for RCU.--- */
---rcu_read_lock_bh();
but we still see the issue "BUG: sleeping function called from invalid cont=
ext":
[ 91.015989] BUG: sleeping function called from invalid context at
include/linux/skbuff.h:786
[ 91.117096] in_atomic(): 1, irqs_disabled(): 0, pid: 1865, name: NMTX_T1=
842
[ 91.200461] Call Trace:
[ 91.229672] [ec58bbd0] [c000789c] show_stack+0x78/0x18c (unreliable)
[ 91.305791] [ec58bc10] [c0022900] __might_sleep+0x100/0x118
[ 91.372524] [ec58bc20] [c029f8d8] dpa_tx+0x128/0x758
[ 91.431957] [ec58bc80] [c02d78ec] dev_hard_start_xmit+0x424/0x588
[ 91.504952] [ec58bcc0] [c02d7ab0] dev_queue_xmit+0x60/0x3ac
[ 91.571692] [ec58bcf0] [c0338d54] packet_sendmsg+0x8c4/0x988
[ 91.639457] [ec58bd70] [c02c3838] sock_sendmsg+0x90/0xb4
[ 91.703066] [ec58be40] [c02c4420] sys_sendto+0xdc/0x120
[ 91.765646] [ec58bf10] [c02c57d0] sys_socketcall+0x148/0x210
[ 91.833420] [ec58bf40] [c001084c] ret_from_syscall+0x0/0x3c
[ 91.900153] --- Exception: c01 at 0x4824df00
[ 91.900157] LR =3D 0x4828a030
The FreeScale patch that introduced this code was created by Andy
Fleming [off-list ref] (Put in CC).
The purpose of the patch is:
"
Subject: [PATCH] net: Add support for handling queueing in hardware
The QDisc code does a bunch of locking which is unnecessary if
you have hardware which handles all of the queueing. Add
support for this, and skip over all of the queueing code if
the feature is enabled on a given device.
"
Ronny
but we still see the issue "BUG: sleeping function called from invalid context":
[ 91.015989] BUG: sleeping function called from invalid context at
include/linux/skbuff.h:786
[ 91.117096] in_atomic(): 1, irqs_disabled(): 0, pid: 1865, name: NMTX_T1842
[ 91.200461] Call Trace:
[ 91.229672] [ec58bbd0] [c000789c] show_stack+0x78/0x18c (unreliable)
[ 91.305791] [ec58bc10] [c0022900] __might_sleep+0x100/0x118
[ 91.372524] [ec58bc20] [c029f8d8] dpa_tx+0x128/0x758
Because this dpa driver's transmit method is doing something else that
is not allowed in software interrupt context.
You must remove all things that might sleep in this driver's
->ndo_start_xmit method, and I do mean everything.
but we still see the issue "BUG: sleeping function called from invalid context":
[ 91.015989] BUG: sleeping function called from invalid context at
include/linux/skbuff.h:786
[ 91.117096] in_atomic(): 1, irqs_disabled(): 0, pid: 1865, name: NMTX_T1842
[ 91.200461] Call Trace:
[ 91.229672] [ec58bbd0] [c000789c] show_stack+0x78/0x18c (unreliable)
[ 91.305791] [ec58bc10] [c0022900] __might_sleep+0x100/0x118
[ 91.372524] [ec58bc20] [c029f8d8] dpa_tx+0x128/0x758
Because this dpa driver's transmit method is doing something else that
is not allowed in software interrupt context.
You must remove all things that might sleep in this driver's
->ndo_start_xmit method, and I do mean everything.
Also this whole HW QOS feature bit facility is beyond bogus.
What if the user enables a qdisc that the hardware can't handle, or a
configuration of a hw supported qdisc that the hardware can't support?
What if I have packet classification and packet actions enabled in the
packet scheduler?
These changes are terrible, and we really need you guys to sort out
your problems with these changes yoursleves because your wounds are
entirely self-inflicted and totally not our problem.
From: Eric Dumazet <hidden> Date: 2011-07-12 15:28:09
Le mardi 12 juillet 2011 à 14:03 +0200, Ronny Meeus a écrit :
Sorry for not mentioning we were using a patched kernel.
I was not aware that the code involved was patched by the FreeScale
patches we applied. The code found in the stack dumps is not
implemented in FSL specific files.
While reading the code of af_packet I saw that the spin_lock_bh is
used in several places while this is not the case in the tpacket_rcv
function. Since we had a locking issue in that code, I thought that my
patch would be OK.
I was not aware that for that specific function (tpacket_rcv) a
different lock primitive must be used. A suggestion for improvement:
it would be better to document this pre-condition in the code.
After doing the change you proposed our code now looks like:
Please read again my mail :
I said : "doing GFP_KERNEL allocations in dpa_tx() is wrong, for sure."
I dont have this code, but I suspect it's using : skb_copy(skb,
GFP_KERNEL)
Just say no, use GFP_ATOMIC instead.
Real question is : why skb_copy() is done, since its slow as hell.
On Tue, Jul 12, 2011 at 5:27 PM, Eric Dumazet [off-list ref] wrot=
e:
Le mardi 12 juillet 2011 =E0 14:03 +0200, Ronny Meeus a =E9crit :
quoted
Sorry for not mentioning we were using a patched kernel.
I was not aware that the code involved was patched by the FreeScale
patches we applied. The code found in the stack dumps is not
implemented in FSL specific files.
While reading the code of af_packet I saw that the spin_lock_bh is
used in several places while this is not the case in the tpacket_rcv
function. Since we had a locking issue in that code, I thought that my
patch would be OK.
I was not aware that for that specific function (tpacket_rcv) a
different lock primitive must be used. A suggestion for improvement:
it would be better to document this pre-condition in the code.
After doing the change you proposed our code now looks like:
Please read again my mail :
I said : "doing GFP_KERNEL allocations in dpa_tx() is wrong, for sure."
I dont have this code, but I suspect it's using : skb_copy(skb,
GFP_KERNEL)
Just say no, use GFP_ATOMIC instead.
Real question is : why skb_copy() is done, since its slow as hell.
After doing this change, I do not see the issue anymore. At least not
for the test I'm doing right now.
After seeing all your comments today, it might be that other issues popup l=
ater.
BTW Are there any good sites (or books) that document this part of the
Linux kernel?
Best regards,
Ronny
Because this dpa driver's transmit method is doing something else that
is not allowed in software interrupt context.
You must remove all things that might sleep in this driver's
->ndo_start_xmit method, and I do mean everything.
Also this whole HW QOS feature bit facility is beyond bogus.
What if the user enables a qdisc that the hardware can't handle, or a
configuration of a hw supported qdisc that the hardware can't support?
What if I have packet classification and packet actions enabled in the
packet scheduler?
These changes are terrible, and we really need you guys to sort out
your problems with these changes yoursleves because your wounds are
entirely self-inflicted and totally not our problem.
Of course, you make very valid points.
I just want to mention that, although you seem to think that we
shouldn't have created this thread in the first place, your and Eric's
remarks have actually helped us in identifying the problem and
increasing our understanding.
So, thanks for that.
Thomas
From: Eric Dumazet <hidden> Date: 2011-07-13 07:39:01
Le mercredi 13 juillet 2011 à 09:20 +0200, Thomas De Schampheleire a
écrit :
I just want to mention that, although you seem to think that we
shouldn't have created this thread in the first place, your and Eric's
remarks have actually helped us in identifying the problem and
increasing our understanding.
Thread was fine IMHO, but should have mentioned the context of "non
standard linux-2.6 or net-next-2.6 trees"
I would like to mention NETIF_F_LLTX existence.
While being marked deprecated, it allows a lockless tx path :
The device transmit handler can then perform custom actions, including
its own locking if needed.
If the user wants a specific Qdisc setup (instead of a no qdisc one), it
still can.
veth, loopback, dummy, macvlan & bond devices use NETIF_F_LLTX for
example.
But ndo_start_xmit() must always be called with BH disabled.