Date:   Tue, 3 May 2022 17:44:28 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "Nambiar, Amritha" <amritha.nambiar@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Mogilappagari, Sudheer" <sudheer.mogilappagari@...el.com>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
        "Sreenivas, Bharathi" <bharathi.sreenivas@...el.com>
Subject: Re: [PATCH net-next 01/11] ice: Add support for classid based queue
 selection

On 2022-05-03 11:47, Jakub Kicinski wrote:
> On Tue, 3 May 2022 06:32:01 -0400 Jamal Hadi Salim wrote:
>> I am on the fence of "six of one, half a dozen of the other" ;->
>>
>> TC classids have *always been used to identify queues* (or hierarchy of
>> but always leading to a single queue). Essentially a classid identity
>> is equivalent to a built-in action which says which queue to pick.
>> The data structure is very much engrained in the tc psyche.
>>
>> When TX side HW queues(IIRC, mqprio) showed up there was ambiguity to
>> distinguish between a s/w queue vs a h/w queue hence queue_mapping
>> which allows us to override the *HW TX queue* selection - or at least
>> that was the intended goal.
>> Note: There are other ways to tell the h/w what queues to use on TX
>> (eg skb->priority) i.e there's no monopoly by queue_mapping.
>>
>> Given lack of s/w queues on RX (hence lack of ambiguity) it seemed
>> natural that the classid could be used to describe the RX queue
>> identity for offload, it's just there.
>> I thought it was brilliant when Amritha first posted.
> 
> Is it just me or is TC generally considered highly confusing?

Hopefully we can discuss what is confusing (otherwise this becomes the
internet troll discussion of "tc sucks").
The fact that folks find ways to use s/w in an unintended ways is
not unique to tc. The architecture is clean.

> IMO using a qdisc cls construct in clsact is only going to add
> to that.
> 
> Assigning classid can still be meaningful on ingress in case
> of a switch where there are actual qdiscs offloaded.
> 
>> I think we should pick the simpler of "half-dozen or six".
>> The posted patch seems driver-only change i.e no core code
>> changes required (which other vendors could follow).
>> But i am not sure if that defines "simpler".
> 
> No core changes is not an argument we should take seriously upstream.
>

I am afraid, that sounds like a blanket statement though.
It should be taken seriously if it helps maintainability
but like i said i wasnt sure having not seen the alternative.

>> BTW:
>> Didnt follow the skb_record_rx_queue() thought - isnt that
>> always set by the driver based on which h/w queue the packet
>> arrived at? Whereas the semantics of this is to tell the h/w
>> what queue to use.
> 
> Overriding the queue mapping in the SW could still be useful
> if TC wants to override the actual queue ID assigned by the NIC.
> 
> This way whether the action gets offloaded or not the resulting
> skb will have the same metadata (in case of offload because it
> came on the right queue and the driver set the mapping, in case
> of sw because we overwrote the mapping).

IIUC, you are suggesting that using the same semantics for ingress and
egress is more intuitive - and that is a fair arguement.
Hope you understand my view when I said it was half a dozen or six
given we have multiple approaches today on egress that signal the
hardware what queue to use (and classid's intent is/was to
select a queue).

cheers,
jamal

Powered by blists - more mailing lists