sfq dump broken in 2.6.27-rc1

sfq dump broken in 2.6.27-rc1

[Stephen Hemminger]

With only one sfq (on eth0), I am getting multiple results from
'tc qdisc ls'

# tc qdisc ls
qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 

Still bisecting, since there is no obvious reason for the sudden borkage.

Re: sfq dump broken in 2.6.27-rc1

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Wed, 6 Aug 2008 18:02:37 -0700

> With only one sfq (on eth0), I am getting multiple results from
> 'tc qdisc ls'
> 
> # tc qdisc ls
> qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> 
> Still bisecting, since there is no obvious reason for the sudden borkage.

Don't bother bisecting, it will take longer than the obvious
set of debug printk's you could add to net/sched/sch_api.c:tc_dump_qdisc().

Please use a "mindful" approach to debugging this instead of
a "mindless" one like bisect :-)

Re: sfq dump broken in 2.6.27-rc1

[Stephen Hemminger]

On Wed, 06 Aug 2008 18:13:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Wed, 6 Aug 2008 18:02:37 -0700
> 
> > With only one sfq (on eth0), I am getting multiple results from
> > 'tc qdisc ls'
> > 
> > # tc qdisc ls
> > qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> > qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> > qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> > qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> > qdisc sfq 8001: dev eth0 root limit 127p quantum 1514b 
> > 
> > Still bisecting, since there is no obvious reason for the sudden borkage.
> 
> Don't bother bisecting, it will take longer than the obvious
> set of debug printk's you could add to net/sched/sch_api.c:tc_dump_qdisc().
> 
> Please use a "mindful" approach to debugging this instead of
> a "mindless" one like bisect :-)

What ever happened to "you broke it, you fix it?"

Re: sfq dump broken in 2.6.27-rc1

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Wed, 6 Aug 2008 20:20:29 -0700

> What ever happened to "you broke it, you fix it?"

What ever happened to debugging things by reading the
code and thinking?

By all means, when I'm dealing with a user who doesn't
know the kernel or networking in particular very well,
going the bisect route is often the way.

But for someone as skilled and knowledgable as you?
Give me a break :-)

[PATCH] net: trap attempts to modify noop qdisc

[Stephen Hemminger]

Since noop qdisc is a singleton, it shouldn't end up with any other
qdisc's on it's list, and it shouldn't be deleted.

Dave, this should help you find the bug.  Any change to root qdisc causes
this to trigger.  I.e doing:
  tc qdisc add dev eth0 root pfifo
causes pfifo to end up on the noop_qdisc->list, which later causes problems.

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 4840aff..57b778f 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -792,8 +792,10 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 				goto err_out3;
 			}
 		}
-		if (parent && !(sch->flags & TCQ_F_INGRESS))
+		if (parent && !(sch->flags & TCQ_F_INGRESS)) {
+			BUG_ON(dev_queue->qdisc == &noop_qdisc);
 			list_add_tail(&sch->list, &dev_queue->qdisc->list);
+		}
 
 		return sch;
 	}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 7cf83b3..5988863 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -547,6 +547,8 @@ static void __qdisc_destroy(struct rcu_head *head)
 
 void qdisc_destroy(struct Qdisc *qdisc)
 {
+	BUG_ON(qdisc == &noop_qdisc);
+
 	if (qdisc->flags & TCQ_F_BUILTIN ||
 	    !atomic_dec_and_test(&qdisc->refcnt))
 		return;
-- 
1.5.4.3

Re: [PATCH] net: trap attempts to modify noop qdisc

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Wed, 6 Aug 2008 23:08:50 -0700

> Since noop qdisc is a singleton, it shouldn't end up with any other
> qdisc's on it's list, and it shouldn't be deleted.
> 
> Dave, this should help you find the bug.

Thanks.

Re: [PATCH] net: trap attempts to modify noop qdisc

[Stephen Hemminger]

On Wed, 06 Aug 2008 23:11:59 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Wed, 6 Aug 2008 23:08:50 -0700
> 
> > Since noop qdisc is a singleton, it shouldn't end up with any other
> > qdisc's on it's list, and it shouldn't be deleted.
> > 
> > Dave, this should help you find the bug.
> 
> Thanks.

I think the root of your problem (bad pun) is that the new code
is assuming that changes to the root are done with parent handle of 0,
but the API is for the parent handle to be TC_H_ROOT (0xFFFFFFFFU).

Re: [PATCH] net: trap attempts to modify noop qdisc

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Wed, 6 Aug 2008 23:15:16 -0700

> I think the root of your problem (bad pun) is that the new code
> is assuming that changes to the root are done with parent handle of 0,
> but the API is for the parent handle to be TC_H_ROOT (0xFFFFFFFFU).

Yes, that's likely it.  I'm playing with some patches now.

BTW, destroying the noop_qdisc is legal, that's why we check
TCF_F_BUILTIN there.

Re: [PATCH] net: trap attempts to modify noop qdisc

[Stephen Hemminger]

On Wed, 06 Aug 2008 23:37:03 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Wed, 6 Aug 2008 23:15:16 -0700
> 
> > On Wed, 06 Aug 2008 23:11:59 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> > > Date: Wed, 6 Aug 2008 23:08:50 -0700
> > > 
> > > > Since noop qdisc is a singleton, it shouldn't end up with any other
> > > > qdisc's on it's list, and it shouldn't be deleted.
> > > > 
> > > > Dave, this should help you find the bug.
> > > 
> > > Thanks.
> > 
> > I think the root of your problem (bad pun) is that the new code
> > is assuming that changes to the root are done with parent handle of 0,
> > but the API is for the parent handle to be TC_H_ROOT (0xFFFFFFFFU).
> 
> This is what I just committed to net-2.6, please give it a whirl.
> 
> pkt_sched: Fix "parent is root" test in qdisc_create().
> 
> As noticed by Stephen Hemminger, the root qdisc is denoted by
> TC_H_ROOT, not zero.
> 
> 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 4840aff..83b23b5 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -792,7 +792,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
>  				goto err_out3;
>  			}
>  		}
> -		if (parent && !(sch->flags & TCQ_F_INGRESS))
> +		if ((parent != TC_H_ROOT) && !(sch->flags & TCQ_F_INGRESS))
>  			list_add_tail(&sch->list, &dev_queue->qdisc->list);
>  
>  		return sch;

Thanks, that fixes it.

You still might want to add a BUG_ON there for anything that is internal qdisc.