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

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

On 2020-07-07 6:05 a.m., Jiri Pirko wrote:
> Fri, Jul 03, 2020 at 01:22:47PM CEST, jhs@mojatatu.com wrote:
>> Hi,
>>
>> Several comments:
>> 1) I agree with previous comments that you should
>> look at incorporating this into skbedit.
>> Unless incorporating into skbedit introduces huge
>> complexity, IMO it belongs there.
>>
>> 2) I think it would make sense to create a skb hash classifier
>> instead of tying this entirely to flower i.e i should not
>> have to change u32 just so i can support hash classification.
> 
> Well, we don't have multiple classifiers for each flower match, we have
> them all in one classifier.

Packet data matches, yes - makes sense. You could argue the same for
the other classifiers.

> It turned out to be very convenient and
> intuitive for people to use one classifier to do the job for them.

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

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.


> Modularity is nice, but useability is I think more important in this
> case. Flower turned out to do good job there.
> 

For humans, agreed everything in one place is convinient.
Note: your arguement could be used for "ls" to include "grep"
functionality because in my scripts I do both most of the time.

cheers,
jamal



> + Nothing stops you from creating separate classifier to match on hash
> as you wanted to :)
> 
>