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

[Vlad Buslov <vladbu@nvidia.com>]

On Mon 01 Nov 2021 at 05:29, Baowen Zheng <baowen.zheng@corigine.com> wrote:
> On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
>>On 2021-10-30 22:27, Baowen Zheng wrote:
>>> Thanks for your review, after some considerarion, I think I understand what
>>you are meaning.
>>>
>>
>>[..]
>>
>>>>>> I know Jamal suggested to have skip_sw for actions, but it
>>>>>> complicates the code and I'm still not entirely understand why it is
>>necessary.
>>>>>
>>>>> If the hardware can independently accept an action offload then
>>>>> skip_sw per action makes total sense. BTW, my understanding is
>>>>
>>>> Example configuration that seems bizarre to me is when offloaded
>>>> shared action has skip_sw flag set but filter doesn't. Then behavior
>>>> of classifier that points to such action diverges between hardware
>>>> and software (different lists of actions are applied). We always try
>>>> to make offloaded TC data path behave exactly the same as software
>>>> and, even though here it would be explicit and deliberate, I don't
>>>> see any practical use-case for this.
>>> We add the skip_sw to keep compatible with the filter flags and give
>>> the user an option to specify if the action should run in software. I
>>> understand what you mean, maybe our example is not proper, we need to
>>> prevent the filter to run in software if the actions it applies is skip_sw, so we
>>need to add more validation to check about this.
>>> Also I think your suggestion makes full sense if there is no use case
>>> to specify the action should not run in sw and indeed it will make our
>>> implement more simple if we omit the skip_sw option.
>>> Jamal, WDYT?
>>
>>
>>Let me use an example to illustrate my concern:
>>
>>#add a policer offload it
>>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>>offloaded 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 so far...
>>#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.
>>
>>If we had added the policer without skip_sw and without skip_hw then i think
>>filter3 should have been legal (we just need to account for stats in_hw vs
>>in_sw).
>>
>>Not sure if that makes sense (and addresses Vlad's earlier comment).
>>
> I think the cases you mentioned make sense to us. But what Vlad concerns is the use
> case as: 
> #add a policer offload it
> tc actions add action police skip_sw rate ... index 20
> #now add filter4 which can't be  offloaded
> tc filter add dev $DEV1 proto ip parent ffff: flower \
> ip_proto tcp action police index 20
> it is possible the filter4 can't be offloaded, then filter4 will run in software,
> should this be legal? 
> Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
> will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
> an action? If a packet matches the filter in software, the action should not be skip_sw. 
> If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work. 
> Of course, we can also keep skip_sw by adding more check to avoid the above case. 
>
> Vlad, I am not sure if I understand your idea correctly. 

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.