Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions

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

On 2021-11-03 08:33, Jamal Hadi Salim wrote:
> On 2021-11-03 07:30, Baowen Zheng wrote:
>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>
>>> [..]
>>>>>>
>>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>>> offload and, consecutively, remove the validation code, not to add
>>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>>> shared action is useful. But I don't have any strong feelings about
>>>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>>
>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>
>>>>> I think these flags add complexity that would be nice to avoid.
>>>>> But if Jamal thinks its necessary, then including the flags
>>>>> implementation is fine by me.
>>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>>> flag for user to specify the action should not run in software?
>>>>
>>>
>>> Just catching up with discussion...
>>> IMO, we need the flag. Oz indicated with requirement to be able to 
>>> identify
>>> the action with an index. So if a specific action is added for 
>>> skip_sw (as
>>> standalone or alongside a filter) then it cant be used for skip_hw. 
>>> To illustrate
>>> using extended example:
>>>
>>> #filter 1, skip_sw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto tcp action police blah index 10
>>>
>>> #filter 2, skip_hw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto udp action police index 10
>>>
>>> Filter2 should be illegal.
>>> And when i dump the actions as so:
>>> tc actions ls action police
>>>
>>> For debugability, I should see index 10 clearly marked with the flag 
>>> as skip_sw
>>>
>>> The other example i gave earlier which showed the sharing of actions:
>>>
>>> #add a policer action and offload it
>>> tc actions add action police skip_sw rate ... index 20 #now add 
>>> filter1 which is
>>> offloaded using offloaded policer tc filter add dev $DEV1 proto ip 
>>> parent ffff:
>>> flower \
>>>      skip_sw ip_proto tcp action police index 20 #add filter2 
>>> likewise offloaded
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto udp action police index 20
>>>
>>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>>
>>> #Now add a filter3 which is s/w only
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto icmp action police index 20
>>>
>>> filter3 should not be allowed.
>> I think the use cases you mentioned above are clear for us. For the case:
>>
>> #add a policer action and offload it
>> tc actions add action police skip_sw rate ... index 20
>> #Now add a filter4 which has no flag
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>       ip_proto icmp action police index 20
>>
>> Is filter4 legal? 
> 
> Yes it is _based on current semantics_.
> The reason is when adding a filter and specifying neither
> skip_sw nor skip_hw it defaults to allowing both.
> i.e is the same as skip_sw|skip_hw. You will need to have
> counters for both s/w and h/w (which i think is taken care of today).
> 
> 

Apologies, i will like to take this one back. Couldnt stop thinking
about it while sipping coffee;->
To be safe that should be illegal. The flags have to match _exactly_
for both  action and filter to make any sense. i.e in the above case
they are not.

cheers,
jamal