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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <486CBE4E.2050901@trash.net>
Date:	Thu, 03 Jul 2008 13:55:58 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Jarek Poplawski <jarkao2@...il.com>
CC:	netdev@...r.kernel.org, devik@....cz
Subject: Re: net-sched 05/05: sch_htb: use dynamic class hash helpers

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@...sh.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html