On Fri, Jan 07, 2005 at 02:15:47AM +0100, Francois Romieu wrote:
Matt Mackall [off-list ref] :
[...]
quoted
I've tinkered with this idea as well, but I'm not convinced it's the
right idea. All current netpoll users (netconsole, kgdb, netdump) are
intrinsically concerned with timeliness of delivery. Queueing packets
^^^^^^^^^^^^^^^
(as well as with reliability of the delivery maybe)
quoted
for later delivery simply doesn't work with the latter two and might
mislead with the former (eg netconsole message and other network
traffic could be reordered).
If kernel/printk.c::vprintk can not take the console semaphore, I
would say that messages are reordered with regard to the order in
which xmit_lock has been taken even without Mark's patch.
So what do you exactly mean by "reordered" ?
printk("debug: at point A");
do_something_that_sends_a_packet_B();
printk("debug: at point C");
Depending on locking and queueing, we might see on our network dump B,
A, C, and wrongly conclude that whatever did B was not between A and C.
That's a bad way for printk to work.
quoted
Also note that there's a closely related class of deadlock that we
can't detect: netconsole-induced recursion on driver-private locks. We
haven't actually seen one of these yet and haven't attempted to audit
for them, but my plan all along has been to treat it as a bug and
hoist offending printks out of the locked regions. Note that this is
really a netconsole-specific problem as the other netpoll clients are
unlikely to have such usage patterns.
Is is still a problem if Mark turns the spinlock in tx_retry_wq() into
an irq safe trylock ?
(driver-private bugs could/will be inherited but fixing these is not
netconsole's job).
The bugs I'm talking about are identical to the xmit_lock deadlock
except with locks we can't see outside the driver. In other words,
this patch addresses the easy part of larger problem by adding a bunch
of complexity that doesn't help in the larger problem. To me, that's a
hint that it's the wrong fix.
quoted
Since the xmit_lock is so similar, it seems things that hit it ought
to get the same treatment. So the rule becomes: no printk with network
driver locks held (public or private). This is obviously broken in the
face of oopsing in the driver, but netconsole can't be expected to
work under such conditions anyway.
I am not convinced that people will be satisfied with a rule which
states that printk _from anywhere_ are lost as soon as a CPU enters
in the xmit_lock zone but, hey, it's just me.
It should only be dropped on the CPU holding the lock, with a loud
warning to follow shortly.
Objection against Ccing further messages to netdev ? It is better indexed
than our mailboxes.
Nope.
--
Mathematics is the supreme nostalgia of our time.
Matt Mackall [off-list ref] :
[...]
printk("debug: at point A");
do_something_that_sends_a_packet_B();
printk("debug: at point C");
Depending on locking and queueing, we might see on our network dump B,
A, C, and wrongly conclude that whatever did B was not between A and C.
That's a bad way for printk to work.
I completely agree that it is not perfect.
However netconsole is currently unable to guarantee that you will
always see both A and B (currently = assuming the skb is dropped
when netconsole fails trylock as suggested by Jamal). So there is
an issue with the reliability of the delivery as well.
I won't push harder on the queuing side as I believe that it will
be possible to add it as an extra choice to the user whatever form
netconsole takes to stop deadlocking.
[...]
The bugs I'm talking about are identical to the xmit_lock deadlock
except with locks we can't see outside the driver. In other words,
Right, it's clearer now. Thanks for the reminder.
User space takes device's private lock -> printks -> netconsole.write
-> hard_start_xmit -> device's private lock -> splat. Same thing from
interrupt context (in_irq() can probably help though).
So we ought to check rtnl_sem as well (dev_base_lock anyone ?).
/me scratches neck...
this patch addresses the easy part of larger problem by adding a bunch
of complexity that doesn't help in the larger problem. To me, that's a
hint that it's the wrong fix.
Too big. It won't bite. :o)
[...]
quoted
I am not convinced that people will be satisfied with a rule which
states that printk _from anywhere_ are lost as soon as a CPU enters
in the xmit_lock zone but, hey, it's just me.
It should only be dropped on the CPU holding the lock, with a loud
warning to follow shortly.
Sorry if I was not clear: "from anywhere" meant printk issued from
any part of the kernel which can interrupt the xmit_locked section
of a qdisc_run(), i.e. printk from irq handlers.
If I read correctly the suggested design, the remaining CPUs should
loop in netpoll_send_skb() when they notice that they can not take
the lock and that their CPU do not own it, right ?
--
Ueimor
On Fri, Jan 07, 2005 at 10:42:54PM +0100, Francois Romieu wrote:
Matt Mackall [off-list ref] :
[...]
quoted
printk("debug: at point A");
do_something_that_sends_a_packet_B();
printk("debug: at point C");
Depending on locking and queueing, we might see on our network dump B,
A, C, and wrongly conclude that whatever did B was not between A and C.
That's a bad way for printk to work.
I completely agree that it is not perfect.
However netconsole is currently unable to guarantee that you will
always see both A and B (currently = assuming the skb is dropped
when netconsole fails trylock as suggested by Jamal). So there is
an issue with the reliability of the delivery as well.
Indeed, and we can't really do much about it generally as we're on a
lossy media. But I think that's less misleading than potential
reordering. I acknowledge that packets can get reordered on the wire
potentially as well but for typical LAN segments, this will be uncommon.
I won't push harder on the queuing side as I believe that it will
be possible to add it as an extra choice to the user whatever form
netconsole takes to stop deadlocking.
I might be willing to compromise on a queue depth of one. Much simpler
code, much less effort to paper over bugs that should be fixed in a
different way.
But then we've got to have a way to disable it for kgdb-over-ethernet
or netdump, where the delayed work simply won't get processed because
the box is stopped. Admittedly trying to trace your way into the
network driver is asking for a beating here, but the general issue is
that netpoll clients can stop interrupt processing and netpoll should
tell such clients that the packets its trying to send aren't going
anywhere rather than quietly scheduling them for later delivery, see?
[...]
quoted
The bugs I'm talking about are identical to the xmit_lock deadlock
except with locks we can't see outside the driver. In other words,
Right, it's clearer now. Thanks for the reminder.
User space takes device's private lock -> printks -> netconsole.write
-> hard_start_xmit -> device's private lock -> splat. Same thing from
interrupt context (in_irq() can probably help though).
So we ought to check rtnl_sem as well (dev_base_lock anyone ?).
/me scratches neck...
quoted
this patch addresses the easy part of larger problem by adding a bunch
of complexity that doesn't help in the larger problem. To me, that's a
hint that it's the wrong fix.
Too big. It won't bite. :o)
[...]
quoted
quoted
I am not convinced that people will be satisfied with a rule which
states that printk _from anywhere_ are lost as soon as a CPU enters
in the xmit_lock zone but, hey, it's just me.
It should only be dropped on the CPU holding the lock, with a loud
warning to follow shortly.
Sorry if I was not clear: "from anywhere" meant printk issued from
any part of the kernel which can interrupt the xmit_locked section
of a qdisc_run(), i.e. printk from irq handlers.
Hrmm. Yes, that's uglier than I realized.
Perhaps there's a way to use the existing IRQ handler reentrancy
avoidance to detect that we might be about to recurse on a lock.
If I read correctly the suggested design, the remaining CPUs should
loop in netpoll_send_skb() when they notice that they can not take
the lock and that their CPU do not own it, right ?
Yep.
--
Mathematics is the supreme nostalgia of our time.
Matt Mackall [off-list ref] :
[...]
quoted
Sorry if I was not clear: "from anywhere" meant printk issued from
any part of the kernel which can interrupt the xmit_locked section
of a qdisc_run(), i.e. printk from irq handlers.
Hrmm. Yes, that's uglier than I realized.
Perhaps there's a way to use the existing IRQ handler reentrancy
avoidance to detect that we might be about to recurse on a lock.
It would help if qdisc_restart() disabled irq until xmit_lock_owner
is set (an extra memory barrier would be required btw).
I'd say to go with in_irq() + try_lock() but one could object that
try_lock() leaves a window where a late thread could steal the
lock.
--
Ueimor