Re: [PATCH net-next v2 0/3] ] TC datapath hash api

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 2020-07-08 10:45 a.m., Jiri Pirko wrote:
> Wed, Jul 08, 2020 at 03:54:14PM CEST, jhs@mojatatu.com wrote:
>> On 2020-07-07 6:05 a.m., Jiri Pirko wrote:


[..]
>> IMO:
>> For this specific case where _offload_ is the main use case i think
>> it is not a good idea because flower on ingress is slow.
> 
> Eh? What do you mean by that?
> 
> 
>> The goal of offloading classifiers to hardware is so one can reduce
>> consumed cpu cycles on the host. If the hardware
>> has done the classification for me, a simple hash lookup of the
>> 32 bit skbhash(similar to fw) in the host would be a lot less
>> compute intensive than running flower's algorithm.
> 
> It is totally up to the driver/fw how they decide to offload flower.
> There are multiple ways. So I don't really follow what do you mean by
> "flower's algorithm"
> 

Nothing to do with how a driver offloads. That part is fine.

By "flower's algorithm" I mean the fact you have to parse and
create the flow cache from scratch on ingress - that slows down
the ingress path. Compare, from cpu cycles pov, to say fw
classifier which dereferences skbmark and uses it as a key
to lookup a hash table.
An skbhash classifier would look the same as fw in its
approach.
subtle point i was making was: if your goal was to save cpu cycles
by offloading the lookup(whose result you then use to do
less work on the host) then you need all the cycles you can
save.

Main point is: classifying based on hash(and for that
matter any other metadata like mark) is needed as a general
utility for the system and should not be only available for
flower. The one big reason we allow all kinds of classifiers
in tc is in the name of "do one thing and do it well".
It is impossible for any one classifier to classify everything
and do a good job at it. For example, I hope you are NEVER
going to add string classification in flower.

Note, what i am describing has precendence:
I can do the same thing with skbmark offloading today.
On ingress I use fw classifier (not u32 or flower).

> 
>>
>> I think there is a line for adding everything in one place,
>> my main concern is that this feature this is needed
>> by all classifiers and not just flower.
> 
> "All" is effectively only flower. Let's be clear about that.
> 

Unless I am misunderstanding, why is it just about flower?
u32 does offload to some hardware. RSS can set this value
and various existing things like XDP, tc ebpf and the action
posted by Ariel.

cheers,
jamal