* [PATCH] netconsole: release the spinlock before __netpoll_cleanup() @ 2013-03-06 14:46 Veaceslav Falico 2013-03-06 14:51 ` Cong Wang 2013-03-07 0:08 ` Neil Horman 0 siblings, 2 replies; 11+ messages in thread From: Veaceslav Falico @ 2013-03-06 14:46 UTC (permalink / raw) To: netdev; +Cc: amwang, davem Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking before __netpoll_cleanup() in netconsole_netdev_event(), however we still might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and add a comment. Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/netconsole.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 37add21..dd62b4c 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, * rtnl_lock already held */ if (nt->np.dev) { + /* + * we still might sleep in + * __netpoll_cleanup(), so release + * the lock + */ + spin_unlock_irqrestore( + &target_list_lock, + flags); __netpoll_cleanup(&nt->np); + spin_lock_irqsave(&target_list_lock, + flags); dev_put(nt->np.dev); nt->np.dev = NULL; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-06 14:46 [PATCH] netconsole: release the spinlock before __netpoll_cleanup() Veaceslav Falico @ 2013-03-06 14:51 ` Cong Wang 2013-03-07 0:08 ` Neil Horman 1 sibling, 0 replies; 11+ messages in thread From: Cong Wang @ 2013-03-06 14:51 UTC (permalink / raw) To: Veaceslav Falico; +Cc: netdev, davem, Neil Horman On Wed, 2013-03-06 at 15:46 +0100, Veaceslav Falico wrote: > Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking > before __netpoll_cleanup() in netconsole_netdev_event(), however we still > might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and > add a comment. > synchronize_srcu() was actually introduced by Neil: commit ca99ca14c95ae49fb4c9cd3abf5f84d11a7e8a61 Author: Neil Horman <nhorman@tuxdriver.com> Date: Tue Feb 5 08:05:43 2013 +0000 netpoll: protect napi_poll and poll_controller during dev_[open| close] Cc'ing him. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-06 14:46 [PATCH] netconsole: release the spinlock before __netpoll_cleanup() Veaceslav Falico 2013-03-06 14:51 ` Cong Wang @ 2013-03-07 0:08 ` Neil Horman 2013-03-07 10:03 ` Veaceslav Falico 1 sibling, 1 reply; 11+ messages in thread From: Neil Horman @ 2013-03-07 0:08 UTC (permalink / raw) To: Veaceslav Falico; +Cc: netdev, amwang, davem On Wed, Mar 06, 2013 at 03:46:43PM +0100, Veaceslav Falico wrote: > Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking > before __netpoll_cleanup() in netconsole_netdev_event(), however we still > might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and > add a comment. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/netconsole.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 37add21..dd62b4c 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, > * rtnl_lock already held > */ > if (nt->np.dev) { > + /* > + * we still might sleep in > + * __netpoll_cleanup(), so release > + * the lock > + */ > + spin_unlock_irqrestore( > + &target_list_lock, > + flags); > __netpoll_cleanup(&nt->np); > + spin_lock_irqsave(&target_list_lock, > + flags); > dev_put(nt->np.dev); > nt->np.dev = NULL; > } > -- > 1.7.1 > Thanks for noticing this Vaeceslav, but you can't just drop and re-acquire the lock like this, as it protect the list_for_each_entry loop that you're in. You can drop the lock in the above if clause, but then, after the nt->np.dev = NULL, go back an re-aquire the lock, and start the for loop. I thought we had already done this for some other purpose in this code using a label and a goto, but I suppose I was mistaken Best Neil > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-07 0:08 ` Neil Horman @ 2013-03-07 10:03 ` Veaceslav Falico 2013-03-07 14:43 ` Neil Horman 2013-03-07 21:14 ` David Miller 0 siblings, 2 replies; 11+ messages in thread From: Veaceslav Falico @ 2013-03-07 10:03 UTC (permalink / raw) To: Neil Horman; +Cc: netdev, amwang, davem On Wed, Mar 06, 2013 at 07:08:24PM -0500, Neil Horman wrote: >On Wed, Mar 06, 2013 at 03:46:43PM +0100, Veaceslav Falico wrote: >> Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking >> before __netpoll_cleanup() in netconsole_netdev_event(), however we still >> might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and >> add a comment. >> >> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >> --- >> drivers/net/netconsole.c | 10 ++++++++++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c >> index 37add21..dd62b4c 100644 >> --- a/drivers/net/netconsole.c >> +++ b/drivers/net/netconsole.c >> @@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, >> * rtnl_lock already held >> */ >> if (nt->np.dev) { >> + /* >> + * we still might sleep in >> + * __netpoll_cleanup(), so release >> + * the lock >> + */ >> + spin_unlock_irqrestore( >> + &target_list_lock, >> + flags); >> __netpoll_cleanup(&nt->np); >> + spin_lock_irqsave(&target_list_lock, >> + flags); >> dev_put(nt->np.dev); >> nt->np.dev = NULL; >> } >> -- >> 1.7.1 >> >Thanks for noticing this Vaeceslav, but you can't just drop and re-acquire the >lock like this, as it protect the list_for_each_entry loop that you're in. You >can drop the lock in the above if clause, but then, after the nt->np.dev = NULL, >go back an re-aquire the lock, and start the for loop. I thought we had already >done this for some other purpose in this code using a label and a goto, but I >suppose I was mistaken You're right, I somehow missed that restart goto, which was removed earlier. Does that feel right (I've also added back the netconsole_target_put()): Subject: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() __netpoll_cleanup() might sleep in synchronize_srcu(), which was added to avoid race in another situation, so we can't call it with the spinlock target_list_lock held. Add spin_unlock/lock before/after it and restart the loop. Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/netconsole.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-) diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 37add21..267c26b 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block *this, goto done; spin_lock_irqsave(&target_list_lock, flags); +restart: list_for_each_entry(nt, &target_list, list) { netconsole_target_get(nt); if (nt->np.dev == dev) { @@ -680,9 +681,21 @@ static int netconsole_netdev_event(struct notifier_block *this, * rtnl_lock already held */ if (nt->np.dev) { + /* + * we still might sleep in + * __netpoll_cleanup(), so release + * the lock and restart + */ + spin_unlock_irqrestore( + &target_list_lock, + flags); __netpoll_cleanup(&nt->np); + spin_lock_irqsave(&target_list_lock, + flags); dev_put(nt->np.dev); nt->np.dev = NULL; + netconsole_target_put(nt); + goto restart; } nt->enabled = 0; stopped = true; -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-07 10:03 ` Veaceslav Falico @ 2013-03-07 14:43 ` Neil Horman 2013-03-07 21:14 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: Neil Horman @ 2013-03-07 14:43 UTC (permalink / raw) To: Veaceslav Falico; +Cc: netdev, amwang, davem On Thu, Mar 07, 2013 at 11:03:25AM +0100, Veaceslav Falico wrote: > On Wed, Mar 06, 2013 at 07:08:24PM -0500, Neil Horman wrote: > >On Wed, Mar 06, 2013 at 03:46:43PM +0100, Veaceslav Falico wrote: > >>Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking > >>before __netpoll_cleanup() in netconsole_netdev_event(), however we still > >>might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and > >>add a comment. > >> > >>Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > >>--- > >> drivers/net/netconsole.c | 10 ++++++++++ > >> 1 files changed, 10 insertions(+), 0 deletions(-) > >> > >>diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > >>index 37add21..dd62b4c 100644 > >>--- a/drivers/net/netconsole.c > >>+++ b/drivers/net/netconsole.c > >>@@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, > >> * rtnl_lock already held > >> */ > >> if (nt->np.dev) { > >>+ /* > >>+ * we still might sleep in > >>+ * __netpoll_cleanup(), so release > >>+ * the lock > >>+ */ > >>+ spin_unlock_irqrestore( > >>+ &target_list_lock, > >>+ flags); > >> __netpoll_cleanup(&nt->np); > >>+ spin_lock_irqsave(&target_list_lock, > >>+ flags); > >> dev_put(nt->np.dev); > >> nt->np.dev = NULL; > >> } > >>-- > >>1.7.1 > >> > >Thanks for noticing this Vaeceslav, but you can't just drop and re-acquire the > >lock like this, as it protect the list_for_each_entry loop that you're in. You > >can drop the lock in the above if clause, but then, after the nt->np.dev = NULL, > >go back an re-aquire the lock, and start the for loop. I thought we had already > >done this for some other purpose in this code using a label and a goto, but I > >suppose I was mistaken > > You're right, I somehow missed that restart goto, which was removed > earlier. Does that feel right (I've also added back the > netconsole_target_put()): > > Subject: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() > > __netpoll_cleanup() might sleep in synchronize_srcu(), which was added to > avoid race in another situation, so we can't call it with the spinlock > target_list_lock held. > > Add spin_unlock/lock before/after it and restart the loop. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/netconsole.c | 13 +++++++++++++ > 1 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 37add21..267c26b 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block *this, > goto done; > spin_lock_irqsave(&target_list_lock, flags); > +restart: > list_for_each_entry(nt, &target_list, list) { > netconsole_target_get(nt); > if (nt->np.dev == dev) { > @@ -680,9 +681,21 @@ static int netconsole_netdev_event(struct notifier_block *this, > * rtnl_lock already held > */ > if (nt->np.dev) { > + /* > + * we still might sleep in > + * __netpoll_cleanup(), so release > + * the lock and restart > + */ > + spin_unlock_irqrestore( > + &target_list_lock, > + flags); > __netpoll_cleanup(&nt->np); > + spin_lock_irqsave(&target_list_lock, > + flags); > dev_put(nt->np.dev); > nt->np.dev = NULL; > + netconsole_target_put(nt); > + goto restart; > } > nt->enabled = 0; > stopped = true; > -- > 1.7.1 > Yes, this looks better, thank you. Acked-by: Neil Horman <nhorman@tuxdriver.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-07 10:03 ` Veaceslav Falico 2013-03-07 14:43 ` Neil Horman @ 2013-03-07 21:14 ` David Miller 2013-03-10 15:25 ` Veaceslav Falico 1 sibling, 1 reply; 11+ messages in thread From: David Miller @ 2013-03-07 21:14 UTC (permalink / raw) To: vfalico; +Cc: nhorman, netdev, amwang From: Veaceslav Falico <vfalico@redhat.com> Date: Thu, 7 Mar 2013 11:03:25 +0100 > @@ -680,9 +681,21 @@ static int netconsole_netdev_event(struct > notifier_block *this, > * rtnl_lock already held > */ > if (nt->np.dev) { > + /* > + * we still might sleep in > + * __netpoll_cleanup(), so release > + * the lock and restart Quite a bit of email corruption of this patch. Also, this code block is probably too deeply indented to be sane, consider creating a small helper function to call instead. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-07 21:14 ` David Miller @ 2013-03-10 15:25 ` Veaceslav Falico 2013-03-11 10:08 ` Veaceslav Falico 0 siblings, 1 reply; 11+ messages in thread From: Veaceslav Falico @ 2013-03-10 15:25 UTC (permalink / raw) To: David Miller; +Cc: nhorman, netdev, amwang On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote: >From: Veaceslav Falico <vfalico@redhat.com> >Date: Thu, 7 Mar 2013 11:03:25 +0100 > >> @@ -680,9 +681,21 @@ static int netconsole_netdev_event(struct >> notifier_block *this, >> * rtnl_lock already held >> */ >> if (nt->np.dev) { >> + /* >> + * we still might sleep in >> + * __netpoll_cleanup(), so release >> + * the lock and restart > >Quite a bit of email corruption of this patch. Sorry, somehow messed it. > >Also, this code block is probably too deeply indented to be sane, >consider creating a small helper function to call instead. It gets quite ugly if I try to move it to another function. However, maybe something like that will work - it's effectively the same code, just that I've moved the long part out of the if () { } block. Looks a lot more readable, though one line still breaks 80chars limit. I've reworked the subject/commit message too. Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic __netpoll_cleanup() is called in netconsole_netdev_event() while holding a spinlock. Release/acquire the spinlock before/after it and restart the loop. Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/netconsole.c | 22 +++++++++++++++------- 1 files changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 37add21..38eaa8c 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block *this, goto done; spin_lock_irqsave(&target_list_lock, flags); +restart: list_for_each_entry(nt, &target_list, list) { netconsole_target_get(nt); if (nt->np.dev == dev) { @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct notifier_block *this, /* * rtnl_lock already held */ - if (nt->np.dev) { - __netpoll_cleanup(&nt->np); - dev_put(nt->np.dev); - nt->np.dev = NULL; + if (!nt->np.dev) { + nt->enabled = 0; + stopped = true; + break; } - nt->enabled = 0; - stopped = true; - break; + /* + * we might sleep in __netpoll_cleanup() + */ + spin_unlock_irqrestore(&target_list_lock, flags); + __netpoll_cleanup(&nt->np); + spin_lock_irqsave(&target_list_lock, flags); + dev_put(nt->np.dev); + nt->np.dev = NULL; + netconsole_target_put(nt); + goto restart; } } netconsole_target_put(nt); -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-10 15:25 ` Veaceslav Falico @ 2013-03-11 10:08 ` Veaceslav Falico 2013-03-11 11:30 ` Neil Horman 0 siblings, 1 reply; 11+ messages in thread From: Veaceslav Falico @ 2013-03-11 10:08 UTC (permalink / raw) To: Veaceslav Falico; +Cc: David Miller, nhorman, netdev, amwang On Sun, Mar 10, 2013 at 4:25 PM, Veaceslav Falico <vfalico@redhat.com> wrote: > On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote: >> ...snip... >> Quite a bit of email corruption of this patch. > > > Sorry, somehow messed it. > > >> >> Also, this code block is probably too deeply indented to be sane, >> consider creating a small helper function to call instead. > > > It gets quite ugly if I try to move it to another function. However, maybe > something like that will work - it's effectively the same code, just that > I've moved the long part out of the if () { } block. Looks a lot more > readable, though one line still breaks 80chars limit. I've reworked the > subject/commit message too. > > Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic > > __netpoll_cleanup() is called in netconsole_netdev_event() while holding a > spinlock. Release/acquire the spinlock before/after it and restart the > > loop. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/netconsole.c | 22 +++++++++++++++------- > 1 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 37add21..38eaa8c 100644 > > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block > *this, > goto done; > spin_lock_irqsave(&target_list_lock, flags); > +restart: > list_for_each_entry(nt, &target_list, list) { > netconsole_target_get(nt); > if (nt->np.dev == dev) { > @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct > notifier_block *this, > /* > > * rtnl_lock already held > */ > - if (nt->np.dev) { > - __netpoll_cleanup(&nt->np); > - dev_put(nt->np.dev); > - nt->np.dev = NULL; > + if (!nt->np.dev) { > + nt->enabled = 0; > + stopped = true; > + break; > } > - nt->enabled = 0; > - stopped = true; > - break; > + /* > + * we might sleep in __netpoll_cleanup() > + */ > + spin_unlock_irqrestore(&target_list_lock, > flags); > + __netpoll_cleanup(&nt->np); > + spin_lock_irqsave(&target_list_lock, flags); > + dev_put(nt->np.dev); > + nt->np.dev = NULL; > > + netconsole_target_put(nt); > + goto restart; > } > } > netconsole_target_put(nt); > -- > 1.7.1 Self-NAK this patch, I've triggered another kernel panic with it. Will send another one shortly. Basicly, the whole if (!nt->np.dev) is not needed and nt->enabled=0 should always be set, otherwise we end up with nt->np.dev == NULL and nt->enabled == 1, thus triggering panics in places like write_msg(), where it verifies only if the nt->enabled is true. -- Best regards, Veaceslav Falico ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-11 10:08 ` Veaceslav Falico @ 2013-03-11 11:30 ` Neil Horman 2013-03-11 11:39 ` Veaceslav Falico 0 siblings, 1 reply; 11+ messages in thread From: Neil Horman @ 2013-03-11 11:30 UTC (permalink / raw) To: Veaceslav Falico; +Cc: Veaceslav Falico, David Miller, netdev, amwang On Mon, Mar 11, 2013 at 11:08:02AM +0100, Veaceslav Falico wrote: > On Sun, Mar 10, 2013 at 4:25 PM, Veaceslav Falico <vfalico@redhat.com> wrote: > > On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote: > >> > ...snip... > >> Quite a bit of email corruption of this patch. > > > > > > Sorry, somehow messed it. > > > > > >> > >> Also, this code block is probably too deeply indented to be sane, > >> consider creating a small helper function to call instead. > > > > > > It gets quite ugly if I try to move it to another function. However, maybe > > something like that will work - it's effectively the same code, just that > > I've moved the long part out of the if () { } block. Looks a lot more > > readable, though one line still breaks 80chars limit. I've reworked the > > subject/commit message too. > > > > Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic > > > > __netpoll_cleanup() is called in netconsole_netdev_event() while holding a > > spinlock. Release/acquire the spinlock before/after it and restart the > > > > loop. > > > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > > --- > > drivers/net/netconsole.c | 22 +++++++++++++++------- > > 1 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > > index 37add21..38eaa8c 100644 > > > > --- a/drivers/net/netconsole.c > > +++ b/drivers/net/netconsole.c > > @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block > > *this, > > goto done; > > spin_lock_irqsave(&target_list_lock, flags); > > +restart: > > list_for_each_entry(nt, &target_list, list) { > > netconsole_target_get(nt); > > if (nt->np.dev == dev) { > > @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct > > notifier_block *this, > > /* > > > > * rtnl_lock already held > > */ > > - if (nt->np.dev) { > > - __netpoll_cleanup(&nt->np); > > - dev_put(nt->np.dev); > > - nt->np.dev = NULL; > > + if (!nt->np.dev) { > > + nt->enabled = 0; > > + stopped = true; > > + break; > > } > > - nt->enabled = 0; > > - stopped = true; > > - break; > > + /* > > + * we might sleep in __netpoll_cleanup() > > + */ > > + spin_unlock_irqrestore(&target_list_lock, > > flags); > > + __netpoll_cleanup(&nt->np); > > + spin_lock_irqsave(&target_list_lock, flags); > > + dev_put(nt->np.dev); > > + nt->np.dev = NULL; > > > > + netconsole_target_put(nt); > > + goto restart; > > } > > } > > netconsole_target_put(nt); > > -- > > 1.7.1 > > Self-NAK this patch, I've triggered another kernel panic with it. Will > send another one shortly. Basicly, the whole if (!nt->np.dev) is not > needed and nt->enabled=0 should always be set, otherwise we > end up with nt->np.dev == NULL and nt->enabled == 1, thus > triggering panics in places like write_msg(), where it verifies only > if the nt->enabled is true. > Yup, I think you want to make the nt->enabled and stopped statements unconditional, and precedede the whole block with a if(!nt->np.dev) { continue;} statement. Neil > -- > Best regards, > Veaceslav Falico > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-11 11:30 ` Neil Horman @ 2013-03-11 11:39 ` Veaceslav Falico 2013-03-11 17:58 ` Neil Horman 0 siblings, 1 reply; 11+ messages in thread From: Veaceslav Falico @ 2013-03-11 11:39 UTC (permalink / raw) To: Neil Horman; +Cc: Veaceslav Falico, David Miller, netdev, amwang On Mon, Mar 11, 2013 at 07:30:24AM -0400, Neil Horman wrote: >On Mon, Mar 11, 2013 at 11:08:02AM +0100, Veaceslav Falico wrote: >> On Sun, Mar 10, 2013 at 4:25 PM, Veaceslav Falico <vfalico@redhat.com> wrote: >> > On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote: >> >> >> ...snip... >> >> Quite a bit of email corruption of this patch. >> > >> > >> > Sorry, somehow messed it. >> > >> > >> >> >> >> Also, this code block is probably too deeply indented to be sane, >> >> consider creating a small helper function to call instead. >> > >> > >> > It gets quite ugly if I try to move it to another function. However, maybe >> > something like that will work - it's effectively the same code, just that >> > I've moved the long part out of the if () { } block. Looks a lot more >> > readable, though one line still breaks 80chars limit. I've reworked the >> > subject/commit message too. >> > >> > Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic >> > >> > __netpoll_cleanup() is called in netconsole_netdev_event() while holding a >> > spinlock. Release/acquire the spinlock before/after it and restart the >> > >> > loop. >> > >> > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >> > --- >> > drivers/net/netconsole.c | 22 +++++++++++++++------- >> > 1 files changed, 15 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c >> > index 37add21..38eaa8c 100644 >> > >> > --- a/drivers/net/netconsole.c >> > +++ b/drivers/net/netconsole.c >> > @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block >> > *this, >> > goto done; >> > spin_lock_irqsave(&target_list_lock, flags); >> > +restart: >> > list_for_each_entry(nt, &target_list, list) { >> > netconsole_target_get(nt); >> > if (nt->np.dev == dev) { >> > @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct >> > notifier_block *this, >> > /* >> > >> > * rtnl_lock already held >> > */ >> > - if (nt->np.dev) { >> > - __netpoll_cleanup(&nt->np); >> > - dev_put(nt->np.dev); >> > - nt->np.dev = NULL; >> > + if (!nt->np.dev) { >> > + nt->enabled = 0; >> > + stopped = true; >> > + break; >> > } >> > - nt->enabled = 0; >> > - stopped = true; >> > - break; >> > + /* >> > + * we might sleep in __netpoll_cleanup() >> > + */ >> > + spin_unlock_irqrestore(&target_list_lock, >> > flags); >> > + __netpoll_cleanup(&nt->np); >> > + spin_lock_irqsave(&target_list_lock, flags); >> > + dev_put(nt->np.dev); >> > + nt->np.dev = NULL; >> > >> > + netconsole_target_put(nt); >> > + goto restart; >> > } >> > } >> > netconsole_target_put(nt); >> > -- >> > 1.7.1 >> >> Self-NAK this patch, I've triggered another kernel panic with it. Will >> send another one shortly. Basicly, the whole if (!nt->np.dev) is not >> needed and nt->enabled=0 should always be set, otherwise we >> end up with nt->np.dev == NULL and nt->enabled == 1, thus >> triggering panics in places like write_msg(), where it verifies only >> if the nt->enabled is true. >> >Yup, I think you want to make the nt->enabled and stopped statements >unconditional, and precedede the whole block with a if(!nt->np.dev) { continue;} >statement. I don't see why the statement is needed at all, if we verify it beforehand: 669 list_for_each_entry(nt, &target_list, list) { 670 netconsole_target_get(nt); 671 if (nt->np.dev == dev) { so that effectively it can be null only in case the dev, delivered via netconsole_netdev_event() is null, which is a bug by itself. Or am I missing something? >Neil > >> -- >> Best regards, >> Veaceslav Falico >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup() 2013-03-11 11:39 ` Veaceslav Falico @ 2013-03-11 17:58 ` Neil Horman 0 siblings, 0 replies; 11+ messages in thread From: Neil Horman @ 2013-03-11 17:58 UTC (permalink / raw) To: Veaceslav Falico; +Cc: Veaceslav Falico, David Miller, netdev, amwang On Mon, Mar 11, 2013 at 12:39:14PM +0100, Veaceslav Falico wrote: > On Mon, Mar 11, 2013 at 07:30:24AM -0400, Neil Horman wrote: > >On Mon, Mar 11, 2013 at 11:08:02AM +0100, Veaceslav Falico wrote: > >>On Sun, Mar 10, 2013 at 4:25 PM, Veaceslav Falico <vfalico@redhat.com> wrote: > >>> On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote: > >>>> > >>...snip... > >>>> Quite a bit of email corruption of this patch. > >>> > >>> > >>> Sorry, somehow messed it. > >>> > >>> > >>>> > >>>> Also, this code block is probably too deeply indented to be sane, > >>>> consider creating a small helper function to call instead. > >>> > >>> > >>> It gets quite ugly if I try to move it to another function. However, maybe > >>> something like that will work - it's effectively the same code, just that > >>> I've moved the long part out of the if () { } block. Looks a lot more > >>> readable, though one line still breaks 80chars limit. I've reworked the > >>> subject/commit message too. > >>> > >>> Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic > >>> > >>> __netpoll_cleanup() is called in netconsole_netdev_event() while holding a > >>> spinlock. Release/acquire the spinlock before/after it and restart the > >>> > >>> loop. > >>> > >>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > >>> --- > >>> drivers/net/netconsole.c | 22 +++++++++++++++------- > >>> 1 files changed, 15 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > >>> index 37add21..38eaa8c 100644 > >>> > >>> --- a/drivers/net/netconsole.c > >>> +++ b/drivers/net/netconsole.c > >>> @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block > >>> *this, > >>> goto done; > >>> spin_lock_irqsave(&target_list_lock, flags); > >>> +restart: > >>> list_for_each_entry(nt, &target_list, list) { > >>> netconsole_target_get(nt); > >>> if (nt->np.dev == dev) { > >>> @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct > >>> notifier_block *this, > >>> /* > >>> > >>> * rtnl_lock already held > >>> */ > >>> - if (nt->np.dev) { > >>> - __netpoll_cleanup(&nt->np); > >>> - dev_put(nt->np.dev); > >>> - nt->np.dev = NULL; > >>> + if (!nt->np.dev) { > >>> + nt->enabled = 0; > >>> + stopped = true; > >>> + break; > >>> } > >>> - nt->enabled = 0; > >>> - stopped = true; > >>> - break; > >>> + /* > >>> + * we might sleep in __netpoll_cleanup() > >>> + */ > >>> + spin_unlock_irqrestore(&target_list_lock, > >>> flags); > >>> + __netpoll_cleanup(&nt->np); > >>> + spin_lock_irqsave(&target_list_lock, flags); > >>> + dev_put(nt->np.dev); > >>> + nt->np.dev = NULL; > >>> > >>> + netconsole_target_put(nt); > >>> + goto restart; > >>> } > >>> } > >>> netconsole_target_put(nt); > >>> -- > >>> 1.7.1 > >> > >>Self-NAK this patch, I've triggered another kernel panic with it. Will > >>send another one shortly. Basicly, the whole if (!nt->np.dev) is not > >>needed and nt->enabled=0 should always be set, otherwise we > >>end up with nt->np.dev == NULL and nt->enabled == 1, thus > >>triggering panics in places like write_msg(), where it verifies only > >>if the nt->enabled is true. > >> > >Yup, I think you want to make the nt->enabled and stopped statements > >unconditional, and precedede the whole block with a if(!nt->np.dev) { continue;} > >statement. > > I don't see why the statement is needed at all, if we verify it beforehand: > > 669 list_for_each_entry(nt, &target_list, list) { > 670 netconsole_target_get(nt); > 671 if (nt->np.dev == dev) { > > so that effectively it can be null only in case the dev, delivered via > netconsole_netdev_event() is null, which is a bug by itself. Or am I > missing something? > Honestly, I'm not sure. After I sent this, I took a look at the history to refresh my memory, and I think your right, its not needed. Neil > >Neil > > > >>-- > >>Best regards, > >>Veaceslav Falico > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-03-11 17:58 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-06 14:46 [PATCH] netconsole: release the spinlock before __netpoll_cleanup() Veaceslav Falico 2013-03-06 14:51 ` Cong Wang 2013-03-07 0:08 ` Neil Horman 2013-03-07 10:03 ` Veaceslav Falico 2013-03-07 14:43 ` Neil Horman 2013-03-07 21:14 ` David Miller 2013-03-10 15:25 ` Veaceslav Falico 2013-03-11 10:08 ` Veaceslav Falico 2013-03-11 11:30 ` Neil Horman 2013-03-11 11:39 ` Veaceslav Falico 2013-03-11 17:58 ` Neil Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).