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

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

Hi Ariel,

On 2020-07-05 1:26 p.m., Ariel Levkovich wrote:
> 
> On 7/3/20 7:22 AM, Jamal Hadi Salim wrote:
[..]
> Hi Jamal,
> 
> I agree that using skbedit makes some sense and can provide the same 
> functionality.
> 
> However I believe that from a concept point of view, using it is wrong.
> 
> In my honest opinion, the concept here is to perform some calculation on 
> the packet itself and its headers while the skb->hash field
> 
> is the storage location of the calculation result (in SW).
> 
> Furthermore, looking forward to HW offload support, the HW devices will 
> be offloading the hash calculation and
> 
> not rewriting skb metadata fields. Therefore the action should be the 
> hash, not skbedit.
> 
> Another thing that I can mention, which is kind of related to what I 
> wrote above, is that for all existing skbedit supported fields,
> 
> user typically provides a desired value of his choosing to set to a skb 
> metadata field.
> 
> Here, the value is unknown and probably not a real concern to the user.
> 
> 
> To sum it up, I look at this as performing some operation on the packet 
> rather then just
> 
> setting an skb metadata field and therefore it requires an explicit, new 
> action.
> 
> 
> What do you think?

skbedit is generally the action for any skb metadata modification
(of which hash is one).
Note: We already have skbedit offload for skbmark today.
The hash feature is useful for software as well (as your use case
showed). I agree with you that the majority of the cases are going to
be a computation of some form that results in dynamic skb->hash.
But the hash should be possible to be statically set by a policy.
BTW, nothing in skbedit is against computing what the new metadata
should be.

IMO: A good arguement to not make it part of skbedit is if it adds
unnecessary complexity to skbedit or policy definitions.

> 
>>
>> 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.
>> So policy would be something of the sort:
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 0 proto ip \
>> flower ip_proto tcp \
>> action skbedit hash bpf object-file <file> \
>> action goto chain 2
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 2 proto ip \
>> handle 0x0 skbhash  flowid 1:11 mask 0xf  \
>> action mirred egress redirect dev ens1f0_1
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 2 proto ip \
>> handle 0x1 skbhash  flowid 1:11 mask 0xf  \
>> action mirred egress redirect dev ens1f0_2
>>
>> IOW, we maintain current modularity as opposed
>> to dumping everything into flower.
>> Ive always wanted to write the skbhash classifier but
>> time was scarce. At one point i had some experiment
>> where I would copy skb hash into mark in the driver
>> and use fw classifier for further processing.
>> It was ugly.
> 
> I agree but perhaps we should make it a separate effort and not block 
> this one.
> 
> I still think we should have support via flower. This is the HW offload 
> path eventually.
> 

My main concern is modularity and the tc principle of doing small
things (and in principle doing them well).
Flower is becoming the sink for everything hardware
offload but f.e u32 also does h/w offload as well and we dont
want to limit it to just those two classifiers for the future...

Note: Flower is not very good performance-wise in the ingress in
s/ware. Something that is more specialized like the way skb mark fw
classifier is will be a lot more efficient. One good reason to make
hardware[1] do the hard work is to save the cyles in the host.
So to me adding to flower does not help that cause.

cheers,
jamal

[1] Whether the hash is set by RSS or an offloaded classifier or
shows up in some simple pkt header (IFE original patch had skb
hash being  set in one machine and transported across machines
for use in a remote machine - that setup is in use in production).

cheers,
jamal