On 2021-11-03 08:33, Jamal Hadi Salim wrote:

On 2021-11-03 07:30, Baowen Zheng wrote:

quoted

On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:

quoted

On 2021-11-03 03:57, Baowen Zheng wrote:

quoted

On November 2, 2021 8:40 PM, Simon Horman wrote:

quoted

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

quoted

On Mon 01 Nov 2021 at 05:29, Baowen Zheng

[..]

quoted

quoted

quoted

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