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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jul 09, 2020 at 01:00:26PM CEST, jhs@mojatatu.com wrote:
>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

Could you point to the specific code please?

The skb->hash is only accessed if the user sets it up for matching.
I don't understand what slowdown you are talking about :/


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

Sure. That classifier can exist, no problem. At the same time, flower
can match on it as well. There are already multiple examples of
classifiers matching on the same thing. I don't see any problem there.


>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