[PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[Jarek Poplawski]

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

Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[Stephen Hemminger]

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>

Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[Jarek Poplawski]

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.

Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[Stephen Hemminger]

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

Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[David Miller]

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.

Re: [PATCH 2.6.19-rc4-git10][PKT_SCHED] sch_htb: INIT_HLIST_NODE after hlist_del()

[Jarek Poplawski]

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.