netdev - Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 May 2022 06:32:01 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "Nambiar, Amritha" <amritha.nambiar@...el.com>
Cc:     "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-04-29 22:42, Jakub Kicinski wrote:
> On Sat, 30 Apr 2022 02:00:05 +0000 Nambiar, Amritha wrote:
>> IIUC, currently the action skbedit queue_mapping is for transmit queue selection,
>> and the bound checking is w.r.to dev->real_num_tx_queues. Also, based on my
>> discussion with Alex (https://www.spinics.net/lists/netdev/msg761581.html), it
>> looks like this currently applies at the qdisc enqueue stage and not at the
>> classifier level.
> 
> They both apply at enqueue stage, AFAIU. Setting classid on ingress
> does exactly nothing, no? :)
> 
> Neither is perfect, at least skbedit seems more straightforward.
> I suspect modern DC operator may have little familiarity with classful
> qdiscs and what classid is. Plus, again, you're assuming mqprio's
> interpretation like it's a TC-wide thing.
> 
> skbedit OTOH is used with a clsact qdisc.
> 
> Also it would be good if what we did had some applicability to SW.
> Maybe extend skbedit with a way of calling skb_record_rx_queue()?

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.

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

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.

cheers,
jamal