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

On Mon 01 Nov 2021 at 05:29, Baowen Zheng [off-list ref] wrote:

On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:

quoted

On 2021-10-30 22:27, Baowen Zheng wrote:

quoted

Thanks for your review, after some considerarion, I think I understand what

you are meaning.

quoted

[..]

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

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.