netdev - Re: [PATCH net-next v3 2/2] net: sched: add Flow Queue PIE packet scheduler

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] [day] [month] [year] [list]

Date:   Mon, 13 Jan 2020 19:04:26 +0530
From:   Gautam Ramakrishnan <gautamramk@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        "Mohit P. Tahiliani" <tahiliani@...k.edu.in>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        "David S . Miller" <davem@...emloft.net>,
        Dave Taht <dave.taht@...il.com>,
        Leslie Monis <lesliemonis@...il.com>,
        "Sachin D . Patil" <sdp.sachin@...il.com>,
        "V . Saicharan" <vsaicharan1998@...il.com>,
        Mohit Bhasi <mohitbhasi1998@...il.com>
Subject: Re: [PATCH net-next v3 2/2] net: sched: add Flow Queue PIE packet scheduler

Hello Jakub and Toke,

Thanks for the comments. We shall make the required changes and
resubmit v4 once we get some more feedback from the mailing list,
probably by tomorrow.

Thank You,
Gautam

On 1/13/20, Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> Jakub Kicinski <kuba@...nel.org> writes:
>
>> On Mon, 13 Jan 2020 12:44:38 +0100, Toke Høiland-Jørgensen wrote:
>>> Jakub Kicinski <kuba@...nel.org> writes:
>>> > On Fri, 10 Jan 2020 11:56:57 +0530, gautamramk@...il.com wrote:
>>> >> From: "Mohit P. Tahiliani" <tahiliani@...k.edu.in>
>>> >>
>>> >> Principles:
>>> >>   - Packets are classified on flows.
>>> >>   - This is a Stochastic model (as we use a hash, several flows might
>>> >>                                 be hashed on the same slot)
>>> >>   - Each flow has a PIE managed queue.
>>> >>   - Flows are linked onto two (Round Robin) lists,
>>> >>     so that new flows have priority on old ones.
>>> >>   - For a given flow, packets are not reordered.
>>> >>   - Drops during enqueue only.
>>> >>   - ECN capability is off by default.
>>> >>   - ECN threshold is at 10% by default.
>>> >>   - Uses timestamps to calculate queue delay by default.
>>> >>
>>> >> Usage:
>>> >> tc qdisc ... fq_pie [ limit PACKETS ] [ flows NUMBER ]
>>> >>                     [ alpha NUMBER ] [ beta NUMBER ]
>>> >>                     [ target TIME us ] [ tupdate TIME us ]
>>> >>                     [ memory_limit BYTES ] [ quantum BYTES ]
>>> >>                     [ ecnprob PERCENTAGE ] [ [no]ecn ]
>>> >>                     [ [no]bytemode ] [ [no_]dq_rate_estimator ]
>>> >>
>>> >> defaults:
>>> >>   limit: 10240 packets, flows: 1024
>>> >>   alpha: 1/8, beta : 5/4
>>> >>   target: 15 ms, tupdate: 15 ms (in jiffies)
>>> >>   memory_limit: 32 Mb, quantum: device MTU
>>> >>   ecnprob: 10%, ecn: off
>>> >>   bytemode: off, dq_rate_estimator: off
>>> >
>>> > Some reviews below, but hopefully someone who knows more about qdiscs
>>> > will still review :)
>>>
>>> I looked it over, and didn't find anything you hadn't already pointed
>>> out below. It's pretty obvious that this started out as a copy of
>>> sch_fq_codel. Which is good, because that's pretty solid. And bad,
>>> because that means it introduces another almost-identical qdisc without
>>> sharing any of the code...
>>>
>>> I think it would be worthwhile to try to consolidate things at some
>>> point. Either by just merging code from fq_{codel,pie}, but another
>>> option would be to express fq_codel and fq_pie using the fq{,_impl}.h
>>> includes. Maybe even sch_cake as well, but that may take a bit more
>>> work. Not sure if we should require this before merging fq_pie, or just
>>> leave it as a possible enhancement for later? WDYT?
>>
>> Tricky :/ No strong opinion on my side. I'm already a little weary of
>> added function calls in the fast path (e.g. pie_drop_early()), but using
>> some static inlines wouldn't hurt... Then again since fq_codel doesn't
>> use fq{,_impl}.h it indeed seems like a bigger project to clean things
>> up.
>
> Yeah, definitely a bigger project; and I do worry about regressions.
> Especially since fq{,_impl}.h relies heavily on indirect calls...
>
>> IMHO if this qdisc works and is useful it could probably be merged as
>> is. Hopefully we can get an opinion on this from Stephen or others.
>
> OK, fine with me :)
>
> -Toke
>
>


-- 
-------------
Gautam |