Re: [PATCH iproute2-next v2] tc: add skip_hw and skip_sw to control action offload

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

On 2022-02-02 04:37, Baowen Zheng wrote:
> Hi Roi:
> Thanks for bring this to us, please see the inline comments.
> 
>> On 2022-02-02 10:39 AM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-01-31 9:40 PM, Jamal Hadi Salim wrote:
>>>> On 2022-01-26 08:41, Victor Nogueira wrote:
>>>>> On Wed, Jan 26, 2022 at 3:55 AM Baowen Zheng
>>>>> <baowen.zheng@corigine.com> wrote:
>>>>>>
>>>>>> Add skip_hw and skip_sw flags for user to control whether offload
>>>>>> action to hardware.
>>>>>>
>>>>>> Also we add hw_count to show how many hardwares accept to offload
>>>>>> the action.
>>>>>>
>>>>>> Change man page to describe the usage of skip_sw and skip_hw flag.
>>>>>>
>>>>>> An example to add and query action as below.
>>>>>>
>>>>>> $ tc actions add action police rate 1mbit burst 100k index 100
>>>>>> skip_sw
>>>>>>
>>>>>> $ tc -s -d actions list action police total acts 1
>>>>>>       action order 0:  police 0x64 rate 1Mbit burst 100Kb mtu 2Kb
>>>>>> action reclassify overhead 0b linklayer ethernet
>>>>>>       ref 1 bind 0  installed 2 sec used 2 sec
>>>>>>       Action statistics:
>>>>>>       Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>>>>>>       backlog 0b 0p requeues 0
>>>>>>       skip_sw in_hw in_hw_count 1
>>>>>>       used_hw_stats delayed
>>>>>>
>>>>>> Signed-off-by: baowen zheng <baowen.zheng@corigine.com>
>>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>>>>>
>>>>> I applied this version, tested it and can confirm the breakage in
>>>>> tdc is gone.
>>>>> Tested-by: Victor Nogueira <victor@mojatatu.com>
>>>>
>>>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>>>
>>>> cheers,
>>>> jamal
>>>
>>>
>>> Hi Sorry for not catching this early enough but I see an issue now
>>> with this patch. adding an offload tc rule and dumping it shows
>>> actions not_in_hw.
>>>
>>> example rule in_hw and action marked as not_in_hw
>>>
>>> filter parent ffff: protocol arp pref 8 flower chain 0 handle 0x1
>>> dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50
>>>     eth_type arp
>>>     in_hw in_hw_count 1
>>>           action order 1: gact action drop
>>>            random type none pass val 0
>>>            index 2 ref 1 bind 1
>>>           not_in_hw
>>>           used_hw_stats delayed
>>>
>>>
>>> so the action was not created/offloaded outside the filter but it is
>>> acting as offloaded.
> Hi Roi, the flag in_hw and not_in_hw in action section describes if the action is offloaded as an action independent of any filter. So the actions created along with the filter will be marked with not_in_hw.

Probably the language usage is causing the confusion and I missed
this detail in the output as well. Let me see if i can break this down.

Either both action and  filter are in h/w or they are not. i.e

action in h/w  + filter in h/w == GOOD
action in h/w  + filter in s/w == BAD
action in s/w  + filter in h/w == BAD
action in s/w  + filter in s/w == GOOD

The kernel patches did have those rules in place - and Baowen added
tdc tests to check for this.

Now on the workflow:
1) If you add an action independently to offload before you add a filter
when you dump actions it should say "skip_sw, ref 1 bind 0"
i.e information is sufficient here to know that the action is offloaded
but there is no filter attached.

2) If you bind this action after to a filter which _has to be offloaded_
(otherwise the filter will be rejected) then when you dump the actions
you should see "skip_sw ref 2 bind 1"; when you dump the filter you
should see the same on the filter.

3) If you create a skip_sw filter without step #1 then when you dump
you should see "skip_sw ref 1 bind 1" both when dumping in
IOW, the not_in_hw is really unnecessary.

So why not just stick with skip_sw and not add some new language?

cheers,
jamal