Date:   Tue, 7 Jul 2020 12:05:56 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Ariel Levkovich <lariel@...lanox.com>, netdev@...r.kernel.org,
        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

Fri, Jul 03, 2020 at 01:22:47PM CEST, jhs@...atatu.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. It turned out to be very convenient and
intuitive for people to use one classifier to do the job for them.
Modularity is nice, but useability is I think more important in this
case. Flower turned out to do good job there.

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


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