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

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 13 Jan 2020 12:44:38 +0100, Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> > On Fri, 10 Jan 2020 11:56:57 +0530, gautamramk@gmail.com wrote:  
> >> From: "Mohit P. Tahiliani" <tahiliani@nitk.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.

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.