netdev - Re: [PATCH 1/2] Stochastic Fair Blue queue discipline, take two

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]

Date:	Sat, 24 May 2008 03:44:55 +0200
From:	Juliusz Chroboczek <jch@....jussieu.fr>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] Stochastic Fair Blue queue discipline, take two

On 9 April 2008, I wrote

>> The following is a second take on the sfb qdisc...

On 13 April, Patrick McHardy was kind enough to review my patch in
Message-ID <4801B574.3050406@...sh.net>, which you'll find on

  http://mid.gmane.org/4801B574.3050406@trash.net

Sorry for the delay, but I really didn't have any time to work on fun
stuff since then.

There are quite a few things in Patrick's message that I don't understand.

>> +struct tc_sfb_qopt {
...
> I'd suggest to use nested attributes, even if you currently don't
> have plans to add new members. The uglyness necessary to do this
> later on is not really worth the small saving in netlink bandwidth.

Could you please clarify what you mean?  Perhaps by pointing me to
some code?

>> +	int i;

> Please use unsigned types where possible.

Is that a hard requirement?  (I happen to prefer int, which I find
more readable.)

>> +	u16 minqlen = (u16)(~0);

> Unnecessary cast and unnecessary parens.

I realise that, but I dislike having non-trivial operations
(truncation) being done by the assignment operation.  Is that a hard
requirement?

>> +	child = sfb_create_dflt(sch, limit);
>> +	if (child == NULL)
>> +		return -ENOMEM;
>> +
>> +	sch_tree_lock(sch);
>> +	if (child) {

> child == NULL is impossible ehere since its checked above. Replacing
> the inner qdisc shouldn't be done unconditionally though, the default
> should only be a default for the initial qdisc.

Please clarify.  This is code that was copied verbatim from sch_red,
and I do not understand it.

Perhaps it'd be better if you made the needed changes to sch_red first?

>> +	q->qdisc = &noop_qdisc;

> The noop_qdisc assignment looks unnecessary, its replaced unconditionally.

Again, this is code copied verbatim from sch_red.  Please make the
changes there first, I'm a little nervous about modifying code I don't
understand.

>> +	struct tc_sfb_qopt opt = { .rehash_interval = q->rehash_interval,
>> +				   .db_interval = q->db_interval,
>> +				   .hash_type = q->hash_type,
>> +				   .limit = q->limit,
>> +				   .max = q->max,
>> +				   .target = q->target,
>> +				   .increment = q->increment,
>> +				   .decrement = q->decrement,
>> +				   .penalty_rate = q->penalty_rate,
>> +				   .penalty_burst = q->penalty_burst,
>> +	};

> Please reindent this correctly by using two tabs (same for other
> C99 initializers).

Please clarify, and point me to a suitable example.  I'm just
following the style in sch_red.

Thanks for your help,

                                        Juliusz
--
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