Message-ID: <8ea64f66-8966-0f19-e329-1c0e5dc4d6d4@mellanox.com>
Date:   Sun, 5 Jul 2020 13:26:35 -0400
From:   Ariel Levkovich <lariel@...lanox.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org
Cc:     jiri@...nulli.us, jiri@...lanox.com, kuba@...nel.org,
        xiyou.wangcong@...il.com, ast@...nel.org, daniel@...earbox.net
Subject: Re: [PATCH net-next v2 0/3] ] TC datapath hash api


On 7/3/20 7:22 AM, Jamal Hadi Salim 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.

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?


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


Regards,

Ariel


> cheers,
> jamal
>
> On 2020-07-01 2:47 p.m., Ariel Levkovich wrote:
>> Supporting datapath hash allows user to set up rules that provide
>> load balancing of traffic across multiple vports and for ECMP path
>> selection while keeping the number of rule at minimum.
>>
>> Instead of matching on exact flow spec, which requires a rule per
>> flow, user can define rules based on hashing on the packet headers
>> and distribute the flows to different buckets. The number of rules
>> in this case will be constant and equal to the number of buckets.
>>
>> The datapath hash functionality is achieved in two steps -
>> performing the hash action and then matching on the result, as
>> part of the packet's classification.
>>
>> The api allows user to define a filter with a tc hash action
>> where the hash function can be standard asymetric hashing that Linux
>> offers or alternatively user can provide a bpf program that
>> performs hash calculation on a packet.
>>
>> Usage is as follows:
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 0 proto ip \
>> flower ip_proto tcp \
>> action hash bpf object-file <file> \
>> action goto chain 2
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 0 proto ip \
>> flower ip_proto udp \
>> action hash bpf asym_l4 basis <basis> \
>> action goto chain 2
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 2 proto ip \
>> flower hash 0x0/0xf  \
>> action mirred egress redirect dev ens1f0_1
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 2 proto ip \
>> flower hash 0x1/0xf  \
>> action mirred egress redirect dev ens1f0_2
>>
>> Ariel Levkovich (3):
>>    net/sched: Introduce action hash
>>    net/flow_dissector: add packet hash dissection
>>    net/sched: cls_flower: Add hash info to flow classification
>>
>>   include/linux/skbuff.h              |   4 +
>>   include/net/act_api.h               |   2 +
>>   include/net/flow_dissector.h        |   9 +
>>   include/net/tc_act/tc_hash.h        |  22 ++
>>   include/uapi/linux/pkt_cls.h        |   4 +
>>   include/uapi/linux/tc_act/tc_hash.h |  32 +++
>>   net/core/flow_dissector.c           |  17 ++
>>   net/sched/Kconfig                   |  11 +
>>   net/sched/Makefile                  |   1 +
>>   net/sched/act_hash.c                | 389 ++++++++++++++++++++++++++++
>>   net/sched/cls_api.c                 |   1 +
>>   net/sched/cls_flower.c              |  16 ++
>>   12 files changed, 508 insertions(+)
>>   create mode 100644 include/net/tc_act/tc_hash.h
>>   create mode 100644 include/uapi/linux/tc_act/tc_hash.h
>>   create mode 100644 net/sched/act_hash.c
>>
>

Powered by blists - more mailing lists