[PATCH]: Schedule correct qdisc in watchdog.

[PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

Jarek, I bet this is what was causing the crash you were trying to fix
yesterday.

But that's just my newbie hunch :-)

pkt_sched: Never scheduler non-root qdiscs.

The qdisc watchdogs can be attached to any qdisc,
not just the root, so make sure we schedule the
correct one.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c8dc72e..98c0084 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -426,7 +426,7 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
 
 	wd->qdisc->flags &= ~TCQ_F_THROTTLED;
 	smp_wmb();
-	__netif_schedule(wd->qdisc);
+	__netif_schedule(qdisc_root(wd->qdisc));
 
 	return HRTIMER_NORESTART;
 }

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 01:39:54AM -0700, David Miller wrote:
> 
> Jarek, I bet this is what was causing the crash you were trying to fix
> yesterday.
> 
> But that's just my newbie hunch :-)

Yes, this was one of the reasons, but not the only one, and my oldbie
version of this fix could be found here as "02-fix1.patch":

http://permalink.gmane.org/gmane.linux.network/103039

It's a bit different, because I think wd->qdisc can probably
point to the noop_qdisc (if it's the root qdisc).

Anyway this kind of change is necessary here, and everywhere where
netif_schedule() could be called (I've found sch_cbq.c only).

Jarek P.

> 
> pkt_sched: Never scheduler non-root qdiscs.
> 
> The qdisc watchdogs can be attached to any qdisc,
> not just the root, so make sure we schedule the
> correct one.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index c8dc72e..98c0084 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -426,7 +426,7 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
>  
>  	wd->qdisc->flags &= ~TCQ_F_THROTTLED;
>  	smp_wmb();
> -	__netif_schedule(wd->qdisc);
> +	__netif_schedule(qdisc_root(wd->qdisc));
>  
>  	return HRTIMER_NORESTART;
>  }

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 18 Aug 2008 09:10:12 +0000

> Yes, this was one of the reasons, but not the only one, and my oldbie
> version of this fix could be found here as "02-fix1.patch":
> 
> http://permalink.gmane.org/gmane.linux.network/103039

This is what I get for failing to be able to follow that
flurry of patches :)  I'll apply your version in the
end, thanks.

> It's a bit different, because I think wd->qdisc can probably
> point to the noop_qdisc (if it's the root qdisc).

noop_qdisc does not use the watchdogs as far as I can see.

But, if we are in dev_deactivate() state, root qdisc can
be &noop_qdisc and scheduling that is fine and just a nop.

> Anyway this kind of change is necessary here, and everywhere where
> netif_schedule() could be called (I've found sch_cbq.c only).

Indeed.

I can't find any other cases that would matter.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 02:31:05AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 18 Aug 2008 09:10:12 +0000
> 
> > Yes, this was one of the reasons, but not the only one, and my oldbie
> > version of this fix could be found here as "02-fix1.patch":
> > 
> > http://permalink.gmane.org/gmane.linux.network/103039
> 
> This is what I get for failing to be able to follow that
> flurry of patches :)  I'll apply your version in the
> end, thanks.
> 
> > It's a bit different, because I think wd->qdisc can probably
> > point to the noop_qdisc (if it's the root qdisc).
> 
> noop_qdisc does not use the watchdogs as far as I can see.
> 
> But, if we are in dev_deactivate() state, root qdisc can
> be &noop_qdisc and scheduling that is fine and just a nop.

Maybe I wrote this wrong. wd->qdisc stores qdisc from the
qdisc_watchdog_init() time, and this could be &noop_qdisc.
So qdisc_root() would schedule wrong qdisc later. BTW, my
version would probably do the same for root qdisc, but in
these tests there was a problem with leafs.

And of course, scheduling &noop_qdisc isn't bad, until we
sometimes schedule something else too.

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 18 Aug 2008 09:47:27 +0000

> Maybe I wrote this wrong. wd->qdisc stores qdisc from the
> qdisc_watchdog_init() time, and this could be &noop_qdisc.
> So qdisc_root() would schedule wrong qdisc later. BTW, my
> version would probably do the same for root qdisc, but in
> these tests there was a problem with leafs.

qdisc_watchdog_init() is only invoked by:

net/sched/sch_cbq.c:	qdisc_watchdog_init(&q->watchdog, sch);
net/sched/sch_hfsc.c:	qdisc_watchdog_init(&q->watchdog, sch);
net/sched/sch_htb.c:	qdisc_watchdog_init(&q->watchdog, sch);
net/sched/sch_netem.c:	qdisc_watchdog_init(&q->watchdog, sch);
net/sched/sch_tbf.c:	qdisc_watchdog_init(&q->watchdog, sch);

These "q" things are the scheduler private structs, and 'sch' of the
qdisc type indicated by the source file the code in question resides
:-)

This watchdog is different from the TX timeout watchdog which is
implemented in net/sch/sch_generic.c, which you may be confusing this
qdisc_watchdog_init() one with.

The watchdog we are discussing here is purely for qdiscs where time
based events modify qdisc state (such as making new quotas available
for a flow, thus making certain packets eligible for scheduling that
were not beforehand)

So really, it cannot be &noop_qdisc as far as I can see.
:-)

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 03:10:03AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 18 Aug 2008 09:47:27 +0000
> 
> > Maybe I wrote this wrong. wd->qdisc stores qdisc from the
> > qdisc_watchdog_init() time, and this could be &noop_qdisc.
> > So qdisc_root() would schedule wrong qdisc later. BTW, my
> > version would probably do the same for root qdisc, but in
> > these tests there was a problem with leafs.
> 
> qdisc_watchdog_init() is only invoked by:
> 
> net/sched/sch_cbq.c:	qdisc_watchdog_init(&q->watchdog, sch);
> net/sched/sch_hfsc.c:	qdisc_watchdog_init(&q->watchdog, sch);
> net/sched/sch_htb.c:	qdisc_watchdog_init(&q->watchdog, sch);
> net/sched/sch_netem.c:	qdisc_watchdog_init(&q->watchdog, sch);
> net/sched/sch_tbf.c:	qdisc_watchdog_init(&q->watchdog, sch);
> 
> These "q" things are the scheduler private structs, and 'sch' of the
> qdisc type indicated by the source file the code in question resides
> :-)
> 
> This watchdog is different from the TX timeout watchdog which is
> implemented in net/sch/sch_generic.c, which you may be confusing this
> qdisc_watchdog_init() one with.

I don't think I wrote anything which could suggest I confused these
watchdogs.

> 
> The watchdog we are discussing here is purely for qdiscs where time
> based events modify qdisc state (such as making new quotas available
> for a flow, thus making certain packets eligible for scheduling that
> were not beforehand)
> 
> So really, it cannot be &noop_qdisc as far as I can see.
> :-)

Sure, ->sch is never &noop_qdisc, but your qdisc_root(sch) most
probably is at the moment of qdisc_watchdog_init(). My version
can do the same if eg. htb is root qdisc because at the moment
of qdisc_create() qdisc_sleeping could be &noop_qdisc as well.
But, in case we have a qdisc_sleeping already, it should work.

So these patches will stop break things, but these qdisc_watchdog()
calls would be sometimes/always useless.

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 10:35:59AM +0000, Jarek Poplawski wrote:
> On Mon, Aug 18, 2008 at 03:10:03AM -0700, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Mon, 18 Aug 2008 09:47:27 +0000
> > 
> > > Maybe I wrote this wrong. wd->qdisc stores qdisc from the
> > > qdisc_watchdog_init() time, and this could be &noop_qdisc.
> > > So qdisc_root() would schedule wrong qdisc later. BTW, my
> > > version would probably do the same for root qdisc, but in
> > > these tests there was a problem with leafs.
> > 
> > qdisc_watchdog_init() is only invoked by:
> > 
> > net/sched/sch_cbq.c:	qdisc_watchdog_init(&q->watchdog, sch);
> > net/sched/sch_hfsc.c:	qdisc_watchdog_init(&q->watchdog, sch);
> > net/sched/sch_htb.c:	qdisc_watchdog_init(&q->watchdog, sch);
> > net/sched/sch_netem.c:	qdisc_watchdog_init(&q->watchdog, sch);
> > net/sched/sch_tbf.c:	qdisc_watchdog_init(&q->watchdog, sch);
> > 
> > These "q" things are the scheduler private structs, and 'sch' of the
> > qdisc type indicated by the source file the code in question resides
> > :-)
> > 
> > This watchdog is different from the TX timeout watchdog which is
> > implemented in net/sch/sch_generic.c, which you may be confusing this
> > qdisc_watchdog_init() one with.
> 
> I don't think I wrote anything which could suggest I confused these
> watchdogs.
> 
> > 
> > The watchdog we are discussing here is purely for qdiscs where time
> > based events modify qdisc state (such as making new quotas available
> > for a flow, thus making certain packets eligible for scheduling that
> > were not beforehand)
> > 
> > So really, it cannot be &noop_qdisc as far as I can see.
> > :-)
> 
> Sure, ->sch is never &noop_qdisc, but your qdisc_root(sch) most
> probably is at the moment of qdisc_watchdog_init(). My version
> can do the same if eg. htb is root qdisc because at the moment
> of qdisc_create() qdisc_sleeping could be &noop_qdisc as well.
> But, in case we have a qdisc_sleeping already, it should work.
> 
> So these patches will stop break things, but these qdisc_watchdog()
> calls would be sometimes/always useless.

Sooory!!! Forget this all: it looks like your patch is damn right!

Jarek P.

PS: I'm still sleeping...

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Denys Fedoryshchenko]

Just to update. Current net-2.6 with [PATCH]: Schedule correct qdisc in watchdog. , for now only following warning appearing

[  177.061416]
[  177.061416] =============================================
[  177.061416] [ INFO: possible recursive locking detected ]
[  177.061416] 2.6.27-rc3-build-0031-12578-g96d2031-dirty #25
[  177.061416] ---------------------------------------------
[  177.061416] swapper/0 is trying to acquire lock:
[  177.061416]  (&list->lock#2){-+..}, at: [<c026191e>] dev_queue_xmit+0x31c/0x4a0
[  177.061416]
[  177.061416] but task is already holding lock:
[  177.061416]  (&list->lock#2){-+..}, at: [<c0260e5d>] netif_receive_skb+0x271/0x400
[  177.061416]
[  177.061416] other info that might help us debug this:
[  177.061416] 5 locks held by swapper/0:
[  177.061416]  #0:  (rcu_read_lock){..--}, at: [<c025f97b>] net_rx_action+0x54/0x1e5
[  177.061416]  #1:  (rcu_read_lock){..--}, at: [<c0260cf8>] netif_receive_skb+0x10c/0x400
[  177.061416]  #2:  (&list->lock#2){-+..}, at: [<c0260e5d>] netif_receive_skb+0x271/0x400
[  177.061416]  #3:  (&p->tcfc_lock){-+..}, at: [<e0a1b303>] tcf_mirred+0x1f/0x14c [act_mirred]
[  177.061416]  #4:  (rcu_read_lock){..--}, at: [<c0261782>] dev_queue_xmit+0x180/0x4a0
[  177.061416]
[  177.061416] stack backtrace:
[  177.061416] Pid: 0, comm: swapper Not tainted 2.6.27-rc3-build-0031-12578-g96d2031-dirty #25
[  177.061416]  [<c02ba6cf>] ? printk+0xf/0x18
[  177.061416]  [<c013ed12>] __lock_acquire+0xb3a/0x118a
[  177.061416]  [<c013f3aa>] lock_acquire+0x48/0x64
[  177.061416]  [<c026191e>] ? dev_queue_xmit+0x31c/0x4a0
[  177.061416]  [<c02bcba1>] _spin_lock+0x1b/0x2a
[  177.061416]  [<c026191e>] ? dev_queue_xmit+0x31c/0x4a0
[  177.061416]  [<c026191e>] dev_queue_xmit+0x31c/0x4a0
[  177.061416]  [<e0a1b40f>] tcf_mirred+0x12b/0x14c [act_mirred]
[  177.061416]  [<e0a1b2e4>] ? tcf_mirred+0x0/0x14c [act_mirred]
[  177.061416]  [<c02711bb>] tcf_action_exec+0x43/0x72
[  177.061416]  [<e0a4dcd5>] u32_classify+0xf4/0x20b [cls_u32]
[  177.061416]  [<c013dabf>] ? trace_hardirqs_on+0xb/0xd
[  177.061416]  [<c026ebb9>] tc_classify_compat+0x2e/0x5d
[  177.061416]  [<c026ed01>] tc_classify+0x17/0x72
[  177.061416]  [<e0a530b2>] ingress_enqueue+0x1a/0x54 [sch_ingress]
[  177.061416]  [<c0260e7d>] netif_receive_skb+0x291/0x400
[  177.061416]  [<c013dabf>] ? trace_hardirqs_on+0xb/0xd
[  177.061416]  [<c0261060>] process_backlog+0x74/0xcb
[  177.061416]  [<c025f9e2>] net_rx_action+0xbb/0x1e5
[  177.061416]  [<c0126203>] __do_softirq+0x7b/0xf4
[  177.061416]  [<c0126188>] ? __do_softirq+0x0/0xf4
[  177.061416]  [<c01060b3>] do_softirq+0x65/0xb6
[  177.061416]  [<c014a35d>] ? handle_fasteoi_irq+0x0/0xb6
[  177.061416]  [<c0125e28>] irq_exit+0x44/0x79
[  177.061416]  [<c0106038>] do_IRQ+0xae/0xc4
[  177.061416]  [<c0104288>] common_interrupt+0x28/0x30
[  177.061416]  [<c013007b>] ? find_get_pid+0x2e/0x4d
[  177.061416]  [<c0108dc6>] ? default_idle+0x32/0x51
[  177.061416]  [<c01029ee>] cpu_idle+0xbf/0xe1
[  177.061416]  [<c02afefa>] rest_init+0x4e/0x50
[  177.061416]  =======================

On Monday 18 August 2008, Jarek Poplawski wrote:
> --
> 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

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 02:04:55PM +0300, Denys Fedoryshchenko wrote:
> Just to update. Current net-2.6 with [PATCH]: Schedule correct qdisc in watchdog. , for now only following warning appearing

So Denys I see you are tracking it all, thanks.

David, I think it would be nice to merge this "00-prevfixes1.patch"
"pkt_sched: Add lockdep annotation for qdisc locks" from:
http://permalink.gmane.org/gmane.linux.network/103039
as well. (It is available on netdev too.)

Thanks,
Jarek P.

> 
> [  177.061416]
> [  177.061416] =============================================
> [  177.061416] [ INFO: possible recursive locking detected ]
> [  177.061416] 2.6.27-rc3-build-0031-12578-g96d2031-dirty #25
> [  177.061416] ---------------------------------------------
> [  177.061416] swapper/0 is trying to acquire lock:
> [  177.061416]  (&list->lock#2){-+..}, at: [<c026191e>] dev_queue_xmit+0x31c/0x4a0
> [  177.061416]
> [  177.061416] but task is already holding lock:
> [  177.061416]  (&list->lock#2){-+..}, at: [<c0260e5d>] netif_receive_skb+0x271/0x400
> [  177.061416]
> [  177.061416] other info that might help us debug this:
> [  177.061416] 5 locks held by swapper/0:
> [  177.061416]  #0:  (rcu_read_lock){..--}, at: [<c025f97b>] net_rx_action+0x54/0x1e5
> [  177.061416]  #1:  (rcu_read_lock){..--}, at: [<c0260cf8>] netif_receive_skb+0x10c/0x400
> [  177.061416]  #2:  (&list->lock#2){-+..}, at: [<c0260e5d>] netif_receive_skb+0x271/0x400
> [  177.061416]  #3:  (&p->tcfc_lock){-+..}, at: [<e0a1b303>] tcf_mirred+0x1f/0x14c [act_mirred]
> [  177.061416]  #4:  (rcu_read_lock){..--}, at: [<c0261782>] dev_queue_xmit+0x180/0x4a0
> [  177.061416]
> [  177.061416] stack backtrace:
> [  177.061416] Pid: 0, comm: swapper Not tainted 2.6.27-rc3-build-0031-12578-g96d2031-dirty #25
> [  177.061416]  [<c02ba6cf>] ? printk+0xf/0x18
> [  177.061416]  [<c013ed12>] __lock_acquire+0xb3a/0x118a
> [  177.061416]  [<c013f3aa>] lock_acquire+0x48/0x64
> [  177.061416]  [<c026191e>] ? dev_queue_xmit+0x31c/0x4a0
> [  177.061416]  [<c02bcba1>] _spin_lock+0x1b/0x2a
> [  177.061416]  [<c026191e>] ? dev_queue_xmit+0x31c/0x4a0
> [  177.061416]  [<c026191e>] dev_queue_xmit+0x31c/0x4a0
> [  177.061416]  [<e0a1b40f>] tcf_mirred+0x12b/0x14c [act_mirred]
> [  177.061416]  [<e0a1b2e4>] ? tcf_mirred+0x0/0x14c [act_mirred]
> [  177.061416]  [<c02711bb>] tcf_action_exec+0x43/0x72
> [  177.061416]  [<e0a4dcd5>] u32_classify+0xf4/0x20b [cls_u32]
> [  177.061416]  [<c013dabf>] ? trace_hardirqs_on+0xb/0xd
> [  177.061416]  [<c026ebb9>] tc_classify_compat+0x2e/0x5d
> [  177.061416]  [<c026ed01>] tc_classify+0x17/0x72
> [  177.061416]  [<e0a530b2>] ingress_enqueue+0x1a/0x54 [sch_ingress]
> [  177.061416]  [<c0260e7d>] netif_receive_skb+0x291/0x400
> [  177.061416]  [<c013dabf>] ? trace_hardirqs_on+0xb/0xd
> [  177.061416]  [<c0261060>] process_backlog+0x74/0xcb
> [  177.061416]  [<c025f9e2>] net_rx_action+0xbb/0x1e5
> [  177.061416]  [<c0126203>] __do_softirq+0x7b/0xf4
> [  177.061416]  [<c0126188>] ? __do_softirq+0x0/0xf4
> [  177.061416]  [<c01060b3>] do_softirq+0x65/0xb6
> [  177.061416]  [<c014a35d>] ? handle_fasteoi_irq+0x0/0xb6
> [  177.061416]  [<c0125e28>] irq_exit+0x44/0x79
> [  177.061416]  [<c0106038>] do_IRQ+0xae/0xc4
> [  177.061416]  [<c0104288>] common_interrupt+0x28/0x30
> [  177.061416]  [<c013007b>] ? find_get_pid+0x2e/0x4d
> [  177.061416]  [<c0108dc6>] ? default_idle+0x32/0x51
> [  177.061416]  [<c01029ee>] cpu_idle+0xbf/0xe1
> [  177.061416]  [<c02afefa>] rest_init+0x4e/0x50
> [  177.061416]  =======================
> 
> On Monday 18 August 2008, Jarek Poplawski wrote:
> > --
> > 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
> 
> 

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 11:20:22AM +0000, Jarek Poplawski wrote:
> On Mon, Aug 18, 2008 at 02:04:55PM +0300, Denys Fedoryshchenko wrote:
> > Just to update. Current net-2.6 with [PATCH]: Schedule correct qdisc in watchdog. , for now only following warning appearing
> 
> So Denys I see you are tracking it all, thanks.
> 
> David, I think it would be nice to merge this "00-prevfixes1.patch"
> "pkt_sched: Add lockdep annotation for qdisc locks" from:
> http://permalink.gmane.org/gmane.linux.network/103039
> as well. (It is available on netdev too.)
> 

Or I'll better resend this, because of some problems with this link.
Denys, please apply this for testing until it's merged.

Jarek P.

------------------> (resend)

pkt_sched: Add lockdep annotation for qdisc locks

Qdisc locks are initialized in the same function, qdisc_alloc(), so
lockdep can't distinguish tx qdisc lock from rx and reports "possible
recursive locking detected" when both these locks are taken eg. while
using act_mirred with ifb. This looks like a false positive. Anyway,
after this patch these locks will be reported more exactly.


Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 net/sched/sch_api.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c25465e..bf84181 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -27,6 +27,7 @@
 #include <linux/kmod.h>
 #include <linux/list.h>
 #include <linux/hrtimer.h>
+#include <linux/lockdep.h>
 
 #include <net/net_namespace.h>
 #include <net/sock.h>
@@ -707,6 +708,10 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 	return err;
 }
 
+/* lockdep annotation is needed for ingress; egress gets it only for name */
+static struct lock_class_key qdisc_tx_lock;
+static struct lock_class_key qdisc_rx_lock;
+
 /*
    Allocate and initialize new qdisc.
 
@@ -767,6 +772,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 	if (handle == TC_H_INGRESS) {
 		sch->flags |= TCQ_F_INGRESS;
 		handle = TC_H_MAKE(TC_H_INGRESS, 0);
+		lockdep_set_class(qdisc_lock(sch), &qdisc_rx_lock);
 	} else {
 		if (handle == 0) {
 			handle = qdisc_alloc_handle(dev);
@@ -774,6 +780,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 			if (handle == 0)
 				goto err_out3;
 		}
+		lockdep_set_class(qdisc_lock(sch), &qdisc_tx_lock);
 	}
 
 	sch->handle = handle;
--
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

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Denys Fedoryshchenko]

Patch applied, got another warning.

[  413.646102]
[  413.646102] =========================
[  413.646102] [ BUG: held lock freed! ]
[  413.646102] -------------------------
[  413.646102] pppd/4105 is freeing memory f6fe6e00-f6fe6eff, with a lock 
still held there!
[  413.646102]  (&qdisc_rx_lock){-+..}, at: [<c026e3ab>] 
shutdown_scheduler_queue+0x1c/0x2e
[  413.646102] 3 locks held by pppd/4105:
[  413.646102]  #0:  (all_ppp_mutex){--..}, at: [<f8a671ae>] 
ppp_shutdown_interface+0x17/0xb0 [ppp_generic]
[  413.646102]  #1:  (rtnl_mutex){--..}, at: [<c026967a>] rtnl_lock+0xf/0x11
[  413.647102]  #2:  (&qdisc_rx_lock){-+..}, at: [<c026e3ab>] 
shutdown_scheduler_queue+0x1c/0x2e
[  413.647102]
[  413.647102] stack backtrace:
[  413.647102] Pid: 4105, comm: pppd Not tainted 
2.6.27-rc3-build-0031-12578-g96d2031-dirty #32
[  413.647102]  [<c02ba6f7>] ? printk+0xf/0x18
[  413.647102]  [<c013db8f>] debug_check_no_locks_freed+0xce/0xff
[  413.647102]  [<c01643ce>] kfree+0x78/0xc8
[  413.647102]  [<c026e38b>] ? qdisc_destroy+0x8c/0x90
[  413.647102]  [<c026e38b>] qdisc_destroy+0x8c/0x90
[  413.647102]  [<c026e3b2>] shutdown_scheduler_queue+0x23/0x2e
[  413.647102]  [<c026e3fa>] dev_shutdown+0x3d/0x59
[  413.647102]  [<c0262240>] rollback_registered+0x128/0x1b1
[  413.647102]  [<c02622fb>] unregister_netdevice+0x32/0x6e
[  413.647102]  [<c0262349>] unregister_netdev+0x12/0x1a
[  413.647102]  [<f8a671f3>] ppp_shutdown_interface+0x5c/0xb0 [ppp_generic]
[  413.647102]  [<f8a6742c>] ppp_release+0x2c/0x5a [ppp_generic]
[  413.647102]  [<c0167db8>] __fput+0x99/0x136
[  413.647102]  [<c0167e6c>] fput+0x17/0x19
[  413.647102]  [<c016593b>] filp_close+0x47/0x51
[  413.647102]  [<c01659b8>] sys_close+0x73/0xad
[  413.647102]  [<c0103861>] sysenter_do_call+0x12/0x35
[  413.647102]  =======================

On Monday 18 August 2008, Jarek Poplawski wrote:

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 03:45:29PM +0300, Denys Fedoryshchenko wrote:
> Patch applied, got another warning.

I hope I'll figure this out before evening. Probably it's safer
to stop testing for a while.

Thanks,
Jarek P.

> 
> [  413.646102]
> [  413.646102] =========================
> [  413.646102] [ BUG: held lock freed! ]
> [  413.646102] -------------------------
> [  413.646102] pppd/4105 is freeing memory f6fe6e00-f6fe6eff, with a lock 
> still held there!
> [  413.646102]  (&qdisc_rx_lock){-+..}, at: [<c026e3ab>] 
> shutdown_scheduler_queue+0x1c/0x2e
> [  413.646102] 3 locks held by pppd/4105:
> [  413.646102]  #0:  (all_ppp_mutex){--..}, at: [<f8a671ae>] 
> ppp_shutdown_interface+0x17/0xb0 [ppp_generic]
> [  413.646102]  #1:  (rtnl_mutex){--..}, at: [<c026967a>] rtnl_lock+0xf/0x11
> [  413.647102]  #2:  (&qdisc_rx_lock){-+..}, at: [<c026e3ab>] 
> shutdown_scheduler_queue+0x1c/0x2e
> [  413.647102]
> [  413.647102] stack backtrace:
> [  413.647102] Pid: 4105, comm: pppd Not tainted 
> 2.6.27-rc3-build-0031-12578-g96d2031-dirty #32
> [  413.647102]  [<c02ba6f7>] ? printk+0xf/0x18
> [  413.647102]  [<c013db8f>] debug_check_no_locks_freed+0xce/0xff
> [  413.647102]  [<c01643ce>] kfree+0x78/0xc8
> [  413.647102]  [<c026e38b>] ? qdisc_destroy+0x8c/0x90
> [  413.647102]  [<c026e38b>] qdisc_destroy+0x8c/0x90
> [  413.647102]  [<c026e3b2>] shutdown_scheduler_queue+0x23/0x2e
> [  413.647102]  [<c026e3fa>] dev_shutdown+0x3d/0x59
> [  413.647102]  [<c0262240>] rollback_registered+0x128/0x1b1
> [  413.647102]  [<c02622fb>] unregister_netdevice+0x32/0x6e
> [  413.647102]  [<c0262349>] unregister_netdev+0x12/0x1a
> [  413.647102]  [<f8a671f3>] ppp_shutdown_interface+0x5c/0xb0 [ppp_generic]
> [  413.647102]  [<f8a6742c>] ppp_release+0x2c/0x5a [ppp_generic]
> [  413.647102]  [<c0167db8>] __fput+0x99/0x136
> [  413.647102]  [<c0167e6c>] fput+0x17/0x19
> [  413.647102]  [<c016593b>] filp_close+0x47/0x51
> [  413.647102]  [<c01659b8>] sys_close+0x73/0xad
> [  413.647102]  [<c0103861>] sysenter_do_call+0x12/0x35
> [  413.647102]  =======================
> 
> On Monday 18 August 2008, Jarek Poplawski wrote:

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 18 Aug 2008 12:58:05 +0000

> On Mon, Aug 18, 2008 at 03:45:29PM +0300, Denys Fedoryshchenko wrote:
> > Patch applied, got another warning.
> 
> I hope I'll figure this out before evening. Probably it's safer
> to stop testing for a while.

We have to put the kfree() of the qdisc back into an RCU handler,
that's all.

I'll cook up a patch for that.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 04:56:38PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 18 Aug 2008 12:58:05 +0000
> 
> > On Mon, Aug 18, 2008 at 03:45:29PM +0300, Denys Fedoryshchenko wrote:
> > > Patch applied, got another warning.
> > 
> > I hope I'll figure this out before evening. Probably it's safer
> > to stop testing for a while.
> 
> We have to put the kfree() of the qdisc back into an RCU handler,
> that's all.

As a matter of fact, I still have some doubts about this. Top level
qdiscs must be deactivated before destroy and during this process we
make sure nothing can use them anymore. So, since this all is under
rtnl_lock(), I wonder if we really need this qdisc root_lock around
qdisc_destroy() for root qdiscs at all.

Maybe there are some common lists which depend on this and rtnl_lock
isn't enough for them. If so, maybe it's easier to change locking in
these places. But, of course, I can miss something.

I'm not against RCU here if it's really needed. Otherwise, this
destroying in softirq context, without rtnl_lock() looks like a
potential obstacle for the future.

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 19 Aug 2008 05:37:45 +0000

> As a matter of fact, I still have some doubts about this. Top level
> qdiscs must be deactivated before destroy and during this process we
> make sure nothing can use them anymore. So, since this all is under
> rtnl_lock(), I wonder if we really need this qdisc root_lock around
> qdisc_destroy() for root qdiscs at all.

Read the read of your inbox :-)

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Tue, Aug 19, 2008 at 05:37:45AM +0000, Jarek Poplawski wrote:
> On Mon, Aug 18, 2008 at 04:56:38PM -0700, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Mon, 18 Aug 2008 12:58:05 +0000
> > 
> > > On Mon, Aug 18, 2008 at 03:45:29PM +0300, Denys Fedoryshchenko wrote:
> > > > Patch applied, got another warning.
> > > 
> > > I hope I'll figure this out before evening. Probably it's safer
> > > to stop testing for a while.
> > 
> > We have to put the kfree() of the qdisc back into an RCU handler,
> > that's all.
> 
> As a matter of fact, I still have some doubts about this. Top level
> qdiscs must be deactivated before destroy and during this process we
> make sure nothing can use them anymore. So, since this all is under
> rtnl_lock(), I wonder if we really need this qdisc root_lock around
> qdisc_destroy() for root qdiscs at all.
> 
> Maybe there are some common lists which depend on this and rtnl_lock
> isn't enough for them. If so, maybe it's easier to change locking in
> these places. But, of course, I can miss something.
> 
> I'm not against RCU here if it's really needed. Otherwise, this
> destroying in softirq context, without rtnl_lock() looks like a
> potential obstacle for the future.

Hmm.. I see it is considered in another messages - I've to do some
reading then.

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

Denys Fedoryshchenko wrote, On 08/18/2008 02:45 PM:

> Patch applied, got another warning.
> 
> [  413.646102]
> [  413.646102] =========================
> [  413.646102] [ BUG: held lock freed! ]
> [  413.646102] -------------------------
> [  413.646102] pppd/4105 is freeing memory f6fe6e00-f6fe6eff, with a lock 
> still held there!
> [  413.646102]  (&qdisc_rx_lock){-+..}, at: [<c026e3ab>] 
> shutdown_scheduler_queue+0x1c/0x2e
...

Alas I can't find anything better for this than the previous patch
named "03-fix_patch_2.patch" in your repository. I attach it below
for any case (apply after all you currently had - there will be a
little offset warning while patching). Alternatively, you can wait
for David's opinion.

Jarek P.

---

 include/linux/netdevice.h |    1 +
 include/net/sch_generic.h |    5 ++++-
 net/core/dev.c            |    1 +
 3 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 488c56e..7041c2c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -445,6 +445,7 @@ struct netdev_queue {
 	struct net_device	*dev;
 	struct Qdisc		*qdisc;
 	unsigned long		state;
+	spinlock_t		qdisc_lock;
 	spinlock_t		_xmit_lock;
 	int			xmit_lock_owner;
 	struct Qdisc		*qdisc_sleeping;
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a7abfda..f84e96c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -185,7 +185,10 @@ static inline struct qdisc_skb_cb *qdisc_skb_cb(struct sk_buff *skb)
 
 static inline spinlock_t *qdisc_lock(struct Qdisc *qdisc)
 {
-	return &qdisc->q.lock;
+	if (unlikely(qdisc->flags & TCQ_F_BUILTIN))
+		return &qdisc->q.lock;
+
+	return &qdisc->dev_queue->qdisc_lock;
 }
 
 static inline struct Qdisc *qdisc_root(struct Qdisc *qdisc)
diff --git a/net/core/dev.c b/net/core/dev.c
index 600bb23..9ec20e0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3859,6 +3859,7 @@ static void __netdev_init_queue_locks_one(struct net_device *dev,
 					  void *_unused)
 {
 	spin_lock_init(&dev_queue->_xmit_lock);
+	spin_lock_init(&dev_queue->qdisc_lock);
 	netdev_set_xmit_lockdep_class(&dev_queue->_xmit_lock, dev->type);
 	dev_queue->xmit_lock_owner = -1;
 }

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Denys Fedoryshchenko]

> Alas I can't find anything better for this than the previous patch
> named "03-fix_patch_2.patch" in your repository. I attach it below
> for any case (apply after all you currently had - there will be a
> little offset warning while patching). Alternatively, you can wait
> for David's opinion.
>
> Jarek P.
>
Tested 1 hour, peak time, alternative and regular shaper - no warning, no 
oops, no panic. Seems all fine for now. I will do more intensive tests 
tomorrow, including backbone units.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 18 Aug 2008 11:35:31 +0000

> pkt_sched: Add lockdep annotation for qdisc locks
> 
> Qdisc locks are initialized in the same function, qdisc_alloc(), so
> lockdep can't distinguish tx qdisc lock from rx and reports "possible
> recursive locking detected" when both these locks are taken eg. while
> using act_mirred with ifb. This looks like a false positive. Anyway,
> after this patch these locks will be reported more exactly.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied, thanks Jarek.

I suspect we need to apply this lock annotation to the locks
in the static qdiscs &noop_qdisc and &noqueue_qdisc.

Could you prepare paatch for that for me?

Thanks again.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 08:54:27PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 18 Aug 2008 11:35:31 +0000
> 
> > pkt_sched: Add lockdep annotation for qdisc locks
> > 
> > Qdisc locks are initialized in the same function, qdisc_alloc(), so
> > lockdep can't distinguish tx qdisc lock from rx and reports "possible
> > recursive locking detected" when both these locks are taken eg. while
> > using act_mirred with ifb. This looks like a false positive. Anyway,
> > after this patch these locks will be reported more exactly.
> > 
> > Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> Applied, thanks Jarek.
> 
> I suspect we need to apply this lock annotation to the locks
> in the static qdiscs &noop_qdisc and &noqueue_qdisc.
> 

I guess, we currently don't need this. At least ingress checks and
skipps acting for &noop_qdisc. I'd prefer to see some lockdep warning
first, or maybe you could show some scenario?

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 19 Aug 2008 06:59:58 +0000

> I guess, we currently don't need this. At least ingress checks and
> skipps acting for &noop_qdisc. I'd prefer to see some lockdep warning
> first, or maybe you could show some scenario?

No, I don't, good point.  Let's leave this for now.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[Jarek Poplawski]

On Mon, Aug 18, 2008 at 10:43:56AM +0000, Jarek Poplawski wrote:
...
> Sooory!!! Forget this all: it looks like your patch is damn right!

So, I guess, you could merge this patch to net-2.6, so Denys could
test this all later without additional patching?!

Jarek P.

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 18 Aug 2008 11:06:25 +0000

> On Mon, Aug 18, 2008 at 10:43:56AM +0000, Jarek Poplawski wrote:
> ...
> > Sooory!!! Forget this all: it looks like your patch is damn right!
> 
> So, I guess, you could merge this patch to net-2.6, so Denys could
> test this all later without additional patching?!

Here is the final patch I put into net-2.6, thanks.

pkt_sched: Never schedule non-root qdiscs.

Based upon initial discovery and patch by Jarek Poplawski.

The qdisc watchdogs can be attached to any qdisc, not just the root,
so make sure we schedule the correct one.

CBQ has a similar bug.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/sch_api.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c8dc72e..98c0084 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -426,7 +426,7 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
 
 	wd->qdisc->flags &= ~TCQ_F_THROTTLED;
 	smp_wmb();
-	__netif_schedule(wd->qdisc);
+	__netif_schedule(qdisc_root(wd->qdisc));
 
 	return HRTIMER_NORESTART;
 }
-- 
1.5.6.5.GIT

Re: [PATCH]: Schedule correct qdisc in watchdog.

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 18 Aug 2008 20:51:53 -0700 (PDT)

> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 18 Aug 2008 11:06:25 +0000
> 
> > On Mon, Aug 18, 2008 at 10:43:56AM +0000, Jarek Poplawski wrote:
> > ...
> > > Sooory!!! Forget this all: it looks like your patch is damn right!
> > 
> > So, I guess, you could merge this patch to net-2.6, so Denys could
> > test this all later without additional patching?!
> 
> Here is the final patch I put into net-2.6, thanks.
> 
> pkt_sched: Never schedule non-root qdiscs.

Sorry, I posted the wrong patch, which ommitted the sch_cbq.c
part, here is the correct one:

pkt_sched: Never schedule non-root qdiscs.

Based upon initial discovery and patch by Jarek Poplawski.

The qdisc watchdogs can be attached to any qdisc, not just the root,
so make sure we schedule the correct one.

CBQ has a similar bug.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/sch_api.c |    2 +-
 net/sched/sch_cbq.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c8dc72e..98c0084 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -426,7 +426,7 @@ static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
 
 	wd->qdisc->flags &= ~TCQ_F_THROTTLED;
 	smp_wmb();
-	__netif_schedule(wd->qdisc);
+	__netif_schedule(qdisc_root(wd->qdisc));
 
 	return HRTIMER_NORESTART;
 }
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 4e261ce..47ef492 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -654,7 +654,7 @@ static enum hrtimer_restart cbq_undelay(struct hrtimer *timer)
 	}
 
 	sch->flags &= ~TCQ_F_THROTTLED;
-	__netif_schedule(sch);
+	__netif_schedule(qdisc_root(sch));
 	return HRTIMER_NORESTART;
 }
 
-- 
1.5.6.5.GIT