netdev - Re: [Patch net-next 2/4] net_sched: update hierarchical backlog too

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:	Wed, 14 Oct 2015 21:32:06 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [Patch net-next 2/4] net_sched: update hierarchical backlog too

On Wed, Oct 14, 2015 at 5:11 AM, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> On 10/12/15 14:38, Cong Wang wrote:
>>
>> When the bottom qdisc decides to, for example, drop some packet,
>> it calls qdisc_tree_decrease_qlen() to update the queue length
>> for all its ancestors, we need to update the backlog too to
>> keep the stats on root qdisc accurate.
>>
>
>
> There is more than one change in there (the codel change seems
> out of place and i wasnt sure why it was needed).

I thought it is clear that when codel decides to drop some packets
we don't know how many bytes it drops, we only know how many
packets before my patch. For example,

-               qdisc_tree_decrease_qlen(sch, q->cstats.drop_count);
+               qdisc_tree_reduce_backlog(sch, q->cstats.drop_count,
+                                         q->cstats.drop_len);

This clearly means I need some codel stats from codel to pass to
qdisc_tree_reduce_backlog(), this is  why the codel part is
necessary.


> Also it seems possible you are double-dipping in some cases;
> i dont have time to scrutinize - but looking at codel_change() change
> when the queue limit is exceeded you will end up affecting backlog from
> both qdisc_qstats_backlog_dec() and your new
> qdisc_tree_reduce_backlog()

Nope, qdisc_qstats_backlog_dec() decreases the backlog of itself,
qdisc_tree_reduce_backlog() decreases its upper qdiscs'. It is correct
as it was.

Thanks.
--
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