sfq dump broken in 2.6.27-rc1

PATCH In review Stephen Hemminger 2 participants 9 messages Aug 7, 2008 1 message not in the archive

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Stephen Hemminger

author

2008-08-07 01:02 UTC #0 lore raw

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.

David Miller

maintainer

2008-08-07 01:13 UTC #1 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 18:02:37 -0700

Quoted 11 lines

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 :-)

Stephen Hemminger

author

2008-08-07 03:20 UTC #2 lore raw

Quoted 13 lines · On Wed, 06 Aug 2008 18:13:27 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 18:02:37 -0700
Quoted 10 lines
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 

Quoted 7 lines

Quoted 1 line
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?"

David Miller

maintainer

2008-08-07 03:22 UTC #3 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 20:20:29 -0700

Quoted 1 line

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 :-)

Stephen Hemminger

author

2008-08-07 06:08 UTC #4 lore raw

PATCH 1/1 net: trap attempts to modify noop qdisc

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.

net/sched/sch_api.c +3 -1

index 4840aff..57b778f 100644
@@ -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;
 	}

net/sched/sch_generic.c +2 -0

index 7cf83b3..5988863 100644
@@ -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;

Signature · 1 line

1.5.4.3

David Miller

maintainer

2008-08-07 06:11 UTC #5 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 23:08:50 -0700

Quoted 4 lines

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.

Stephen Hemminger

author

2008-08-07 06:15 UTC #6 lore raw

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

Quoted 9 lines · On Wed, 06 Aug 2008 23:11:59 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 23:08:50 -0700
Quoted 4 lines
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).

David Miller

maintainer

2008-08-07 06:18 UTC #7 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 23:15:16 -0700

Quoted 3 lines

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stephen Hemminger

author

2008-08-07 17:22 UTC #9 lore raw

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

Quoted 37 lines · On Wed, 06 Aug 2008 23:37:03 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 23:15:16 -0700
Quoted 9 lines
Quoted 6 lines · On Wed, 06 Aug 2008 23:11:59 -0700 (PDT) David Miller <[email protected]>…
From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Aug 2008 23:08:50 -0700
Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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 <[email protected]>
---
 net/sched/sch_api.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/sched/sch_api.c +0 -0
index 4840aff..83b23b5 100644

net/sched/sch_api.c qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue, · line 798

Quoted 8 lines

net/sched/sch_api.c +1 -1

@@ -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.