Re: net-sched 05/05: sch_htb: use dynamic class hash helpers

[Patrick McHardy <kaber@trash.net>]

Jarek Poplawski wrote:
> Patrick McHardy wrote, On 07/01/2008 04:34 PM:
>
>   
>> net-sched: sch_htb: use dynamic class hash helpers
>>
>> Signed-off-by: Patrick McHardy <kaber@trash.net>
>>     
>
> This patch also looks OK too me, except 2 tiny doubts below:
>
>   
>> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
>> index 879f0b6..d5e4fdb 100644
>> --- a/net/sched/sch_htb.c
>> +++ b/net/sched/sch_htb.c
>> @@ -51,7 +51,6 @@
>>      one less than their parent.
>>  */
>>  
>> -#define HTB_HSIZE 16		/* classid hash size */
>>  static int htb_hysteresis __read_mostly = 0; /* whether to use mode hysteresis for speedup */
>>  #define HTB_VER 0x30011		/* major must be matched with number suplied by TC as version */
>>  
>> @@ -72,8 +71,8 @@ enum htb_cmode {
>>  
>>  /* interior & leaf nodes; props specific to leaves are marked L: */
>>  struct htb_class {
>> +	struct Qdisc_class_common common;
>>     
>
> Hmm... isn't this variable name too "common"? Why not something more
> meaningful like: id, node, head, info etc?
>   
I couldn't come up with something better, your
suggestions are also not much more descriptive
in my opinion. I'm also hoping we can put more
stuff in there and so some consolidation later.

> ...
>   
>> @@ -975,13 +957,14 @@ static unsigned int htb_drop(struct Qdisc *sch)
>>  static void htb_reset(struct Qdisc *sch)
>>  {
>>  	struct htb_sched *q = qdisc_priv(sch);
>> -	int i;
>> -
>> -	for (i = 0; i < HTB_HSIZE; i++) {
>> -		struct hlist_node *p;
>> -		struct htb_class *cl;
>> +	struct Qdisc_class_common *clc;
>> +	struct hlist_node *n;
>> +	struct htb_class *cl;
>> +	unsigned int i;
>>  
>> -		hlist_for_each_entry(cl, p, q->hash + i, hlist) {
>> +	for (i = 0; i < q->clhash.hashsize; i++) {
>> +		hlist_for_each_entry(clc, n, &q->clhash.hash[i], hnode) {
>> +			cl = container_of(clc, struct htb_class, common);
>>     
>
> Why not?:
> 		hlist_for_each_entry(cl, n, &q->clhash.hash[i], common.hnode) {
>
> Of course, this is probably a matter of taste, so I don't persist...

Good point, that is nicer. I'll update the patches.