diff mbox

netpoll: Don't call driver methods from interrupt context.

Message ID 87ha7cwiry.fsf@xmission.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric W. Biederman March 5, 2014, 7:24 p.m. UTC
David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Tue, 04 Mar 2014 16:03:43 -0800
>
>> So I would like some clear guidance.  Will you accept patches to make
>> it safe to call the napi poll routines from hard irq context, or should
>> we simply defer messages prented with netconsole in hard irq context
>> into another context where we can run the napi code?
>> 
>> If there is not a clear way to fix the problems that crop up we should
>> just delete all of the netpoll code altogether, as it seems deadly in
>> it's current form.
>
> Clearly to make netconsole most useful we should synchronously emit
> log messages.
>
> Because what if the system hangs right after this event, but before
> we get back to a "safe" context.
>
> That's one bug that will be a billion times harder to diagnose if
> we defer.

In general I agree.  

The gripping hand for me is kernel/rcu/tree.c:print_cpu_stall() that
generates a warning from irq context on every cpu simultaneously.

Which without netpoll I can debug by just logging into the machine and
dumping dmesg, but with netpoll machine die when the warning is
generarted because of the after the first few messages each additional
message generates a new message.

Now that I have looked closer the printk generating a printk problem
seems to be something that is best solved at the printk level.  So if
you will accept the patches I will proceed to shore up the existing
netpoll implementations.

I am thinking pretty seriously about forcing hard irq context during
netconsole's use of netpoll to ensure that the hard irq context case
actually get's tested.  I need to do some audit's to see if that would
cause any side effects beyond leaving irq's disabled.


Eric


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller March 7, 2014, 7:30 p.m. UTC | #1
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 05 Mar 2014 11:24:33 -0800

> Now that I have looked closer the printk generating a printk problem
> seems to be something that is best solved at the printk level.

I'm not so sure that disallowing printk recursion is necessary.

If you consider an error printk emitted from a device driver's
transmit function during netconsole output, netpoll handles this
transparently already.

Basically, what happens right now in this situation is that netpoll
queues it up when recursion is detected, and delayed work is scheduled
to process such pending packets.

The only issue at hand is the IRQ context bit.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman March 8, 2014, 5:13 a.m. UTC | #2
David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Wed, 05 Mar 2014 11:24:33 -0800
>
>> Now that I have looked closer the printk generating a printk problem
>> seems to be something that is best solved at the printk level.
>
> I'm not so sure that disallowing printk recursion is necessary.
>
> If you consider an error printk emitted from a device driver's
> transmit function during netconsole output, netpoll handles this
> transparently already.
>
> Basically, what happens right now in this situation is that netpoll
> queues it up when recursion is detected, and delayed work is scheduled
> to process such pending packets.

Except that printk does not recurse into netpoll again, printk adds the
message to printk's ring buffer, and then the next the next time through
the loop in console_unlock writes that message out with console_unlock.

I have had warnings from printk kill a couple of machines, which is
largely why I am anxious to fix netpoll.  Further I have experimentally
verified that I can still kill a machine that way in the 3.14-rcX.

> The only issue at hand is the IRQ context bit.

That is the only issue that is a networking stack issue, and I am happy to
focus there.  If we don't get printk's generating warnings the machine
won't lock up.

I am slowly working my way through reading the code and verifying I
really understand what is going on so I can reasonably say the routines
in the appropriate drivers should be safe in hard irq context.

Hopefully I will have patches in the next couple of days.

Eric
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index ba2f5e710af1..aaa9062061c8 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -734,6 +734,7 @@  static void write_msg(struct console *con, const char *msg, unsigned int len)
        unsigned long flags;
        struct netconsole_target *nt;
        const char *tmp;
+       bool hard_irq;
 
        if (oops_only && !oops_in_progress)
                return;
@@ -742,6 +743,9 @@  static void write_msg(struct console *con, const char *msg, unsigned int len)
                return;
 
        spin_lock_irqsave(&target_list_lock, flags);
+       hard_irq = in_irq();
+       if (!hard_irq)
+               irq_enter();
        list_for_each_entry(nt, &target_list, list) {
                netconsole_target_get(nt);
                if (nt->enabled && netif_running(nt->np.dev)) {
@@ -761,6 +765,8 @@  static void write_msg(struct console *con, const char *msg, unsigned int len)
                }
                netconsole_target_put(nt);
        }
+       if (!hard_irq)
+               irq_exit();
        spin_unlock_irqrestore(&target_list_lock, flags);
 }