Re: [PATCH net-next] net:sched: add gkprio scheduler

From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Michel Machado <michel@digirati.com.br>,
	Nishanth Devarajan <ndev2021@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>, David Miller <davem@davemloft.net>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	Cody Doucette <doucette@bu.edu>
Subject: Re: [PATCH net-next] net:sched: add gkprio scheduler
Date: Wed, 9 May 2018 10:43:46 -0400	[thread overview]
Message-ID: <bee1e54d-e2eb-1393-1a4e-d54e731f7bfc@mojatatu.com> (raw)
In-Reply-To: <CAM_iQpXc4PzKZ7aQAp-sno57AEcgx21upJecQ1J1V0VaH1xM_g@mail.gmail.com>

On 08/05/18 10:27 PM, Cong Wang wrote:
> On Tue, May 8, 2018 at 6:29 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> Have you considered using skb->prio instead of peeking into the packet
>> header.
>> Also have you looked at the dsmark qdisc?
>>
> 
> dsmark modifies ds fields, while this one just maps ds fields into
> different queues.
> 

Yeah, I was thinking more of re-using it for the purpose of mapping to
queues - but would require a lot more work.

once skbprio is set by something[1] then this qdisc could be used by
other subsystems (8021q, sockets etc); so i would argue for removal
of the embedded classification and instead maybe writing a simple
extension to skbmod to mark skbprio based on ds.

I find the cleverness in changing the highest/low prios confusing.
It looks error-prone (I guess that is why there is a BUG check)
To the authors: Is there a document/paper on the theory of this thing
as to why no explicit queues are "faster"?

Some other feedback:

1) I agree that using multiple queues as in prio qdisc would make it
more manageable; does not necessarily need to be classful if you
use implicit skbprio classification. i.e on equeue use a priority
map to select a queue; on dequeue always dequeu from highest prio
until it has no more packets to send.

2) Dropping already enqueued packets will not work well for
local feedback (__NET_XMIT_BYPASS return code is about the
packet that has been dropped from earlier enqueueing because
it is lower priority - it does not  signify anything with
current skb to which actually just got enqueud).
Perhaps (off top of my head) is to always enqueue packets on
high priority when their limit is exceeded as long as lower prio has
some space. Means youd have to increment low prio accounting if their
space is used.

cheers,
jamal

[1] something like:
tc filter add match all ip action skbmod inheritdsfield
tc filter add match all ip6 action skbmod inheritdsfield

inheritdsfield maps ds to skb->prioriry