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

[Baowen Zheng <baowen.zheng@corigine.com>]

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. 
This is to be compatible with what we do in Linux upstream 8cbfe93 ("flow_offload: allow user to offload tc action to net device"). 

>>
>> also shouldn't the indent be more 1 space in like random/index to note
>> it's part of the action order 1.
From my environment, I did not find this indent issue, I will make more check to verify.
>>
>> Thanks,
>> Roi
>>
>
>also, not tested. what is printed if match is not supported but uses offloaded
>action?
If match is not supported but uses offloaded action, the match will be marked as not_in_hw and the action will be marked as in_hw since the action is offloaded independent from filter rule.
>
>it could print filter not_in_hw but action in_hw?