* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.