* [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
@ 2006-11-06 11:33 Jarek Poplawski
2006-11-06 17:44 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Jarek Poplawski @ 2006-11-06 11:33 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, David S. Miller
After hlist_del() next and pprev pointers are not NULL
so hlist_unhashed() doesn't work properly.
Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
---
diff -Nurp linux-2.6.19-rc4-git10-/net/sched/sch_htb.c linux-2.6.19-rc4-git10/net/sched/sch_htb.c
--- linux-2.6.19-rc4-git10-/net/sched/sch_htb.c 2006-11-06 11:42:41.000000000 +0100
+++ linux-2.6.19-rc4-git10/net/sched/sch_htb.c 2006-11-06 11:53:15.000000000 +0100
@@ -1284,8 +1284,10 @@ static void htb_destroy_class(struct Qdi
struct htb_class, sibling));
/* note: this delete may happen twice (see htb_delete) */
- if (!hlist_unhashed(&cl->hlist))
+ if (!hlist_unhashed(&cl->hlist)) {
hlist_del(&cl->hlist);
+ INIT_HLIST_NODE(&cl->hlist);
+ }
list_del(&cl->sibling);
if (cl->prio_activity)
@@ -1333,8 +1335,10 @@ static int htb_delete(struct Qdisc *sch,
sch_tree_lock(sch);
/* delete from hash and active; remainder in destroy_class */
- if (!hlist_unhashed(&cl->hlist))
+ if (!hlist_unhashed(&cl->hlist)) {
hlist_del(&cl->hlist);
+ INIT_HLIST_NODE(&cl->hlist);
+ }
if (cl->prio_activity)
htb_deactivate(q, cl);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
2006-11-06 11:33 [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del() Jarek Poplawski
@ 2006-11-06 17:44 ` Stephen Hemminger
2006-11-07 6:49 ` Jarek Poplawski
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2006-11-06 17:44 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: netdev, David S. Miller
On Mon, 6 Nov 2006 12:33:53 +0100
Jarek Poplawski <jarkao2@o2.pl> wrote:
> After hlist_del() next and pprev pointers are not NULL
> so hlist_unhashed() doesn't work properly.
>
>
> Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> ---
>
>
> diff -Nurp linux-2.6.19-rc4-git10-/net/sched/sch_htb.c linux-2.6.19-rc4-git10/net/sched/sch_htb.c
> --- linux-2.6.19-rc4-git10-/net/sched/sch_htb.c 2006-11-06 11:42:41.000000000 +0100
> +++ linux-2.6.19-rc4-git10/net/sched/sch_htb.c 2006-11-06 11:53:15.000000000 +0100
> @@ -1284,8 +1284,10 @@ static void htb_destroy_class(struct Qdi
> struct htb_class, sibling));
>
> /* note: this delete may happen twice (see htb_delete) */
> - if (!hlist_unhashed(&cl->hlist))
> + if (!hlist_unhashed(&cl->hlist)) {
> hlist_del(&cl->hlist);
> + INIT_HLIST_NODE(&cl->hlist);
> + }
why not use hlist_del_init?
> list_del(&cl->sibling);
>
> if (cl->prio_activity)
> @@ -1333,8 +1335,10 @@ static int htb_delete(struct Qdisc *sch,
> sch_tree_lock(sch);
>
> /* delete from hash and active; remainder in destroy_class */
> - if (!hlist_unhashed(&cl->hlist))
> + if (!hlist_unhashed(&cl->hlist)) {
> hlist_del(&cl->hlist);
> + INIT_HLIST_NODE(&cl->hlist);
> + }
>
> if (cl->prio_activity)
> htb_deactivate(q, cl);
--
Stephen Hemminger <shemminger@osdl.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
2006-11-06 17:44 ` Stephen Hemminger
@ 2006-11-07 6:49 ` Jarek Poplawski
2006-11-07 17:50 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Jarek Poplawski @ 2006-11-07 6:49 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, David S. Miller
On Mon, Nov 06, 2006 at 09:44:49AM -0800, Stephen Hemminger wrote:
> On Mon, 6 Nov 2006 12:33:53 +0100
> Jarek Poplawski <jarkao2@o2.pl> wrote:
>
> > After hlist_del() next and pprev pointers are not NULL
> > so hlist_unhashed() doesn't work properly.
> >
> >
> > Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> > ---
> >
> >
> > diff -Nurp linux-2.6.19-rc4-git10-/net/sched/sch_htb.c linux-2.6.19-rc4-git10/net/sched/sch_htb.c
> > --- linux-2.6.19-rc4-git10-/net/sched/sch_htb.c 2006-11-06 11:42:41.000000000 +0100
> > +++ linux-2.6.19-rc4-git10/net/sched/sch_htb.c 2006-11-06 11:53:15.000000000 +0100
> > @@ -1284,8 +1284,10 @@ static void htb_destroy_class(struct Qdi
> > struct htb_class, sibling));
> >
> > /* note: this delete may happen twice (see htb_delete) */
> > - if (!hlist_unhashed(&cl->hlist))
> > + if (!hlist_unhashed(&cl->hlist)) {
> > hlist_del(&cl->hlist);
> > + INIT_HLIST_NODE(&cl->hlist);
> > + }
>
> why not use hlist_del_init?
Yes, this is the question!
As a matter of fact I expected another question. Yesterday
I was short on time so I didn't describe the bug enough.
I'm not sure if you know the problem, so here are more
details (for me problem is 199% repeatable).
After something like this:
# tc qdisc add dev lo root handle 1: htb
# tc class add dev lo parent 1: classid 1:1 htb rate 200kbps
# tc class del dev lo classid 1:1
enter the BUG...
I've found the last command is the culprit and if you do:
# tc qdisc del dev lo root
there is no problem.
And probably it is enough to do the change only in htb_delete
- btw. is this hlist_del really needed there? and shouldn't
all deletions be done after zeroing the refcount? - but you
should know better.
>
> > list_del(&cl->sibling);
> >
> > if (cl->prio_activity)
> > @@ -1333,8 +1335,10 @@ static int htb_delete(struct Qdisc *sch,
> > sch_tree_lock(sch);
> >
> > /* delete from hash and active; remainder in destroy_class */
> > - if (!hlist_unhashed(&cl->hlist))
> > + if (!hlist_unhashed(&cl->hlist)) {
> > hlist_del(&cl->hlist);
> > + INIT_HLIST_NODE(&cl->hlist);
> > + }
> >
> > if (cl->prio_activity)
> > htb_deactivate(q, cl);
Best regards,
Jarek P.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
2006-11-07 6:49 ` Jarek Poplawski
@ 2006-11-07 17:50 ` Stephen Hemminger
2006-11-07 22:55 ` David Miller
2006-11-08 6:36 ` Jarek Poplawski
0 siblings, 2 replies; 6+ messages in thread
From: Stephen Hemminger @ 2006-11-07 17:50 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: netdev, David S. Miller
On Tue, 7 Nov 2006 07:49:43 +0100
Jarek Poplawski <jarkao2@o2.pl> wrote:
> On Mon, Nov 06, 2006 at 09:44:49AM -0800, Stephen Hemminger wrote:
> > On Mon, 6 Nov 2006 12:33:53 +0100
> > Jarek Poplawski <jarkao2@o2.pl> wrote:
> >
> > > After hlist_del() next and pprev pointers are not NULL
> > > so hlist_unhashed() doesn't work properly.
> > >
> > >
> > > Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> > > ---
> > >
> > >
> > > diff -Nurp linux-2.6.19-rc4-git10-/net/sched/sch_htb.c linux-2.6.19-rc4-git10/net/sched/sch_htb.c
> > > --- linux-2.6.19-rc4-git10-/net/sched/sch_htb.c 2006-11-06 11:42:41.000000000 +0100
> > > +++ linux-2.6.19-rc4-git10/net/sched/sch_htb.c 2006-11-06 11:53:15.000000000 +0100
> > > @@ -1284,8 +1284,10 @@ static void htb_destroy_class(struct Qdi
> > > struct htb_class, sibling));
> > >
> > > /* note: this delete may happen twice (see htb_delete) */
> > > - if (!hlist_unhashed(&cl->hlist))
> > > + if (!hlist_unhashed(&cl->hlist)) {
> > > hlist_del(&cl->hlist);
> > > + INIT_HLIST_NODE(&cl->hlist);
> > > + }
> >
> > why not use hlist_del_init?
Your patch duplicated the code in hlist_del_init(). Why not do:
--- a/net/sched/sch_htb.c 2006-11-07 09:48:22.000000000 -0800
+++ b/net/sched/sch_htb.c 2006-11-07 09:49:01.000000000 -0800
@@ -1284,8 +1284,7 @@
struct htb_class, sibling));
/* note: this delete may happen twice (see htb_delete) */
- if (!hlist_unhashed(&cl->hlist))
- hlist_del(&cl->hlist);
+ hlist_del_init(&cl->hlist);
list_del(&cl->sibling);
if (cl->prio_activity)
@@ -1333,8 +1332,7 @@
sch_tree_lock(sch);
/* delete from hash and active; remainder in destroy_class */
- if (!hlist_unhashed(&cl->hlist))
- hlist_del(&cl->hlist);
+ hlist_del_init(&cl->hlist);
if (cl->prio_activity)
htb_deactivate(q, cl);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
2006-11-07 17:50 ` Stephen Hemminger
@ 2006-11-07 22:55 ` David Miller
2006-11-08 6:36 ` Jarek Poplawski
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2006-11-07 22:55 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, netdev
From: Stephen Hemminger <shemminger@osdl.org>
Date: Tue, 7 Nov 2006 09:50:07 -0800
> Your patch duplicated the code in hlist_del_init(). Why not do:
Indeed, this is the patch I will apply.
Thanks Stephen.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()
2006-11-07 17:50 ` Stephen Hemminger
2006-11-07 22:55 ` David Miller
@ 2006-11-08 6:36 ` Jarek Poplawski
1 sibling, 0 replies; 6+ messages in thread
From: Jarek Poplawski @ 2006-11-08 6:36 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, David S. Miller
On Tue, Nov 07, 2006 at 09:50:07AM -0800, Stephen Hemminger wrote:
> On Tue, 7 Nov 2006 07:49:43 +0100
> Jarek Poplawski <jarkao2@o2.pl> wrote:
...
> Your patch duplicated the code in hlist_del_init(). Why not do:
>
> --- a/net/sched/sch_htb.c 2006-11-07 09:48:22.000000000 -0800
> +++ b/net/sched/sch_htb.c 2006-11-07 09:49:01.000000000 -0800
> @@ -1284,8 +1284,7 @@
> struct htb_class, sibling));
>
> /* note: this delete may happen twice (see htb_delete) */
> - if (!hlist_unhashed(&cl->hlist))
> - hlist_del(&cl->hlist);
> + hlist_del_init(&cl->hlist);
> list_del(&cl->sibling);
>
> if (cl->prio_activity)
> @@ -1333,8 +1332,7 @@
> sch_tree_lock(sch);
>
> /* delete from hash and active; remainder in destroy_class */
> - if (!hlist_unhashed(&cl->hlist))
> - hlist_del(&cl->hlist);
> + hlist_del_init(&cl->hlist);
>
> if (cl->prio_activity)
> htb_deactivate(q, cl);
>
I've understood you first suggestion. But after sending
my patch I've found it is also hiding a real problem
of excessive deletion in one and possibly more places.
So probably this should be done the right way and this
hlist_unhashed testing left in BUG_ON only...
Cheers,
Jarek P.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-11-08 6:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-11-06 11:33 [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del() Jarek Poplawski
2006-11-06 17:44 ` Stephen Hemminger
2006-11-07 6:49 ` Jarek Poplawski
2006-11-07 17:50 ` Stephen Hemminger
2006-11-07 22:55 ` David Miller
2006-11-08 6:36 ` Jarek Poplawski
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).