netdev - RE: [PATCH iproute2-next v2] tc: add skip_hw and skip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM5PR1301MB21722969131F70DD7FA350EFE7309@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date:   Fri, 11 Feb 2022 10:01:53 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>, Roi Dayan <roid@...dia.com>,
        Victor Nogueira <victor@...atatu.com>
CC:     David Ahern <dsahern@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        oss-drivers <oss-drivers@...igine.com>,
        Simon Horman <simon.horman@...igine.com>
Subject: RE: [PATCH iproute2-next v2] tc: add skip_hw and skip_sw to control
 action offload

Hi Jamal:
Sorry for the delay of the reply.

On February 2, 2022 7:47 PM, Jamal wrote:
>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@...igine.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@...igine.com>
>>>>>>> Signed-off-by: Simon Horman <simon.horman@...igine.com>
>>>>>>
>>>>>> I applied this version, tested it and can confirm the breakage in
>>>>>> tdc is gone.
>>>>>> Tested-by: Victor Nogueira <victor@...atatu.com>
>>>>>
>>>>> Acked-by: Jamal Hadi Salim <jhs@...atatu.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?
>
If I do not misunderstand, you mean we just show the skip_sw flag and do not show other information(in_hw, not_in_hw and in_hw_count), I think it is reasonable to show the action information as your suggestion if the action is dumped along with the filters. 

But as we discussed previously, we added the flags of skip_hw, skip_sw, in_hw_count mainly for the action dump command(tc -s -d actions list action xxx).
We know that the action can be created with three flags case: skip_sw, skip_hw and no flag.
Then when the actions are dumped independently, the information of skip_hw, skip_sw, in_hw_count will become important for the user to distinguish if the action is offloaded or not. 

So does that mean we need to show different item when the action is dumped independent or along with the filter? 

>cheers,
>jamal