Re: [PATCH net-next v3 2/4] net/sched: Introduce action hash

[Ariel Levkovich <lariel@mellanox.com>]

On 7/15/20 2:49 PM, Daniel Borkmann wrote:
> On 7/15/20 3:30 PM, Ariel Levkovich wrote:
>> On 7/15/20 2:12 AM, Cong Wang wrote:
>>> On Mon, Jul 13, 2020 at 8:17 PM Ariel Levkovich 
>>> <lariel@mellanox.com> wrote:
>>>> On 7/13/20 6:04 PM, Cong Wang wrote:
>>>>> On Sat, Jul 11, 2020 at 2:28 PM Ariel Levkovich 
>>>>> <lariel@mellanox.com> wrote:
>>>>>> Allow user to set a packet's hash value using a bpf program.
>>>>>>
>>>>>> The user provided BPF program is required to compute and return
>>>>>> a hash value for the packet which is then stored in skb->hash.
>>>>> Can be done by act_bpf, right?
>>>> Right. We already agreed on that.
>>>>
>>>> Nevertheless, as I mentioned, act_bpf is not offloadable.
>>>>
>>>> Device driver has no clue what the program does.
>>> What about offloading act_skbedit? You care about offloading
>>> the skb->hash computation, not about bpf.
>>>
>>> Thanks.
>>
>> That's true but act_skedit provides (according to the current design) 
>> hash
>>
>> computation using a kernel implemented algorithm.
>>
>> HW not necessarily can offload this kernel based jhash function and 
>> therefore
>>
>> we introduce the bpf option. With bpf the user can provide an 
>> implemenation
>>
>> of a hash function that the HW can actually offload and that way user
>>
>> maintains consistency between SW hash calculation and HW.
>>
>> For example, in cases where offload is added dynamically as traffic 
>> flows, like
>>
>> in the OVS case, first packets will go to SW and hash will be 
>> calculated on them
>>
>> using bpf that emulates the HW hash so that this packet will get the 
>> same hash
>>
>> result that it will later get in HW when the flow is offloaded.
>>
>>
>> If there's a strong objection to adding a new action,
>>
>> IMO, we can include the bpf option in act_skbedit - action skbedit 
>> hash bpf <bpf.o>
>>
>> What do u think?
>
> Please don't. From a BPF pov this is all very misleading since it 
> might wrongly suggest
> to the user that existing means aka {cls,act}_bpf in tc are not 
> capable of already doing
> this. They are capable for several years already though. Also, it is 
> very confusing that
> act_hash or 'skbedit hash bpf' can do everything that {cls,act}_bpf 
> can do already, so
> much beyond setting a hash value (e.g. you could set tunnel keys etc 
> from there). Given
> act_hash is only about offloading but nothing else, did you consider 
> for the BPF alternative
> to just use plain old classic BPF given you only need to parse the pkt 
> and calc the hash
> val but nothing more?

You can do almost everything with act_bpf and yet there are explicit 
actions to set a tunnel

key and add/remove MPLS header (and more...).

What do u mean by classic BPF? How will that help with the offload?

It will still go via act_bpf without any indication on what type of 
program is this, won't it?

Thanks,

Ariel