On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:

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

quoted

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

..

quoted

quoted

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.

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.