| Message ID | 1643180079-17097-1-git-send-email-baowen.zheng@corigine.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4cd4f127047cb78760677eafbc38f047efeb0a3 |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v2] tc: add skip_hw and skip_sw to control action offload | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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>
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
Hello: This patch was applied to iproute2/iproute2-next.git (main) by David Ahern <dsahern@kernel.org>: On Wed, 26 Jan 2022 14:54:39 +0800 you 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. > > [...] Here is the summary with links: - [iproute2-next,v2] tc: add skip_hw and skip_sw to control action offload https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=f4cd4f127047 You are awesome, thank you!
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. also shouldn't the indent be more 1 space in like random/index to note it's part of the action order 1. Thanks, Roi
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. > > also shouldn't the indent be more 1 space in like random/index to > note it's part of the action order 1. > > Thanks, > Roi > also, not tested. what is printed if match is not supported but uses offloaded action? it could print filter not_in_hw but action in_hw?
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?
On 2022-02-02 11:37 AM, 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. > This is to be compatible with what we do in Linux upstream 8cbfe93 ("flow_offload: allow user to offload tc action to net device"). > I understand it's for the actions offload but there is not confusing output between if actions were created explicitly or by the filter. In the example above the action is "offloaded". the matching and action are both done in hw. maybe if action is created by the filter should not dump in_hw/not_in_hw flags at all. >>> >>> 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. its ok. i saw the indents from different commit and got fixed, I needed to refetch. i dont see the indent issues now. >>> >>> 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?
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
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@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? > 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
On 2022-02-11 05:01, Baowen Zheng wrote: > Hi Jamal: > Sorry for the delay of the reply. > I guess it is my turn to say sorry for the latency ;-> > 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. >>> [..] >> >> 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. > Yes, thats what i am saying - it maintains the existing semantics people are aware of for usability. > 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? > I see your point. I am trying to visualize how we deal with the tri-state in filters and we never considered what you are suggesting. Most people either skip_sw or skip_hw in presence of offloadable hw. In absence of hardware nobody specifies a flag, so nothing is displayed. My eyes are used to how filters look like. Not sure anymore tbh. Roi? cheers, jamal
On 2022-02-16 2:51 PM, Jamal Hadi Salim wrote: > On 2022-02-11 05:01, Baowen Zheng wrote: >> Hi Jamal: >> Sorry for the delay of the reply. >> > > I guess it is my turn to say sorry for the latency ;-> > >> 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. >>>> > > [..] >>> >>> 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. >> > > Yes, thats what i am saying - it maintains the existing semantics people > are aware of for usability. > >> 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? >> > > I see your point. I am trying to visualize how we deal with the > tri-state in filters and we never considered what you are suggesting. > Most people either skip_sw or skip_hw in presence of offloadable hw. > In absence of hardware nobody specifies a flag, so nothing is displayed. > My eyes are used to how filters look like. Not sure anymore tbh. Roi? > Hi, Is the question here if to show different information when actions are dumped independently or with a filter? then I think yes. when actions are dumped as part of the filter skip showing skip_sw/skip_hw/in_hw/not_in_hw flags as it's redundant and it's always whatever the filter state is. I also noticed we can improve extack msgs when a user will try to mix the state like adding a filter without skip_hw flag but use action index that is created with skip_hw. I noticed currently there is no informative extack msg back to the user. Thanks, Roi > cheers, > jamal > >
On February 16, 2022 10:18 PM, Roi wrote: >On 2022-02-16 2:51 PM, Jamal Hadi Salim wrote: >> On 2022-02-11 05:01, Baowen Zheng wrote: >>> Hi Jamal: >>> Sorry for the delay of the reply. >>> >> >> I guess it is my turn to say sorry for the latency ;-> >> >>> 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. >>>>> >> >> [..] >>>> >>>> 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. >>> >> >> Yes, thats what i am saying - it maintains the existing semantics >> people are aware of for usability. >> >>> 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? >>> >> >> I see your point. I am trying to visualize how we deal with the >> tri-state in filters and we never considered what you are suggesting. >> Most people either skip_sw or skip_hw in presence of offloadable hw. >> In absence of hardware nobody specifies a flag, so nothing is displayed. >> My eyes are used to how filters look like. Not sure anymore tbh. Roi? >> > >Hi, > >Is the question here if to show different information when actions are >dumped independently or with a filter? > >then I think yes. when actions are dumped as part of the filter skip showing >skip_sw/skip_hw/in_hw/not_in_hw flags as it's redundant and it's always >whatever the filter state is. > >I also noticed we can improve extack msgs when a user will try to mix the state >like adding a filter without skip_hw flag but use action index that is created >with skip_hw. >I noticed currently there is no informative extack msg back to the user. Thanks Roi and Jamal, we will think about how to display different information in action dump independently or along with the filter. Also for the extack msg enhancement for the user. > >Thanks, >Roi > > >> cheers, >> jamal >> >>
diff --git a/man/man8/tc-actions.8 b/man/man8/tc-actions.8 index 6f1c201..5c399cd 100644 --- a/man/man8/tc-actions.8 +++ b/man/man8/tc-actions.8 @@ -52,6 +52,8 @@ actions \- independently defined actions in tc .I HWSTATSSPEC ] [ .I CONTROL +] [ +.I SKIPSPEC ] .I ACTISPEC @@ -99,6 +101,11 @@ Time since last update. .IR reclassify " | " pipe " | " drop " | " continue " | " ok } +.I SKIPSPEC +:= { +.IR skip_sw " | " skip_hw +} + .I TC_OPTIONS These are the options that are specific to .B tc @@ -270,6 +277,23 @@ Return to the calling qdisc for packet processing, and end classification of this packet. .RE +.TP +.I SKIPSPEC +The +.I SKIPSPEC +indicates how +.B tc +should proceed when executing the action. Any of the following are valid: +.RS +.TP +.B skip_sw +Do not process action by software. If hardware has no offload support for this +action, operation will fail. +.TP +.B skip_hw +Do not process action by hardware. +.RE + .SH SEE ALSO .BR tc (8), .BR tc-bpf (8), diff --git a/tc/m_action.c b/tc/m_action.c index b16882a..b4cf94f 100644 --- a/tc/m_action.c +++ b/tc/m_action.c @@ -51,9 +51,10 @@ static void act_usage(void) " FL := ls | list | flush | <ACTNAMESPEC>\n" " ACTNAMESPEC := action <ACTNAME>\n" " ACTISPEC := <ACTNAMESPEC> <INDEXSPEC>\n" - " ACTSPEC := action <ACTDETAIL> [INDEXSPEC] [HWSTATSSPEC]\n" + " ACTSPEC := action <ACTDETAIL> [INDEXSPEC] [HWSTATSSPEC] [SKIPSPEC]\n" " INDEXSPEC := index <32 bit indexvalue>\n" " HWSTATSSPEC := hw_stats [ immediate | delayed | disabled ]\n" + " SKIPSPEC := [ skip_sw | skip_hw ]\n" " ACTDETAIL := <ACTNAME> <ACTPARAMS>\n" " Example ACTNAME is gact, mirred, bpf, etc\n" " Each action has its own parameters (ACTPARAMS)\n" @@ -245,6 +246,8 @@ int parse_action(int *argc_p, char ***argv_p, int tca_id, struct nlmsghdr *n) goto done0; } else { struct action_util *a = NULL; + int skip_loop = 2; + __u32 flag = 0; if (!action_a2n(*argv, NULL, false)) strncpy(k, "gact", sizeof(k) - 1); @@ -314,13 +317,27 @@ done0: } if (*argv && strcmp(*argv, "no_percpu") == 0) { + flag |= TCA_ACT_FLAGS_NO_PERCPU_STATS; + NEXT_ARG_FWD(); + } + + /* we need to parse twice to fix skip flag out of order */ + while (skip_loop--) { + if (*argv && strcmp(*argv, "skip_sw") == 0) { + flag |= TCA_ACT_FLAGS_SKIP_SW; + NEXT_ARG_FWD(); + } else if (*argv && strcmp(*argv, "skip_hw") == 0) { + flag |= TCA_ACT_FLAGS_SKIP_HW; + NEXT_ARG_FWD(); + } + } + + if (flag) { struct nla_bitfield32 flags = - { TCA_ACT_FLAGS_NO_PERCPU_STATS, - TCA_ACT_FLAGS_NO_PERCPU_STATS }; + { flag, flag }; addattr_l(n, MAX_MSG, TCA_ACT_FLAGS, &flags, sizeof(struct nla_bitfield32)); - NEXT_ARG_FWD(); } addattr_nest_end(n, tail); @@ -396,13 +413,39 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg) strsz, b1, sizeof(b1))); print_nl(); } - if (tb[TCA_ACT_FLAGS]) { - struct nla_bitfield32 *flags = RTA_DATA(tb[TCA_ACT_FLAGS]); + if (tb[TCA_ACT_FLAGS] || tb[TCA_ACT_IN_HW_COUNT]) { + bool skip_hw = false; + if (tb[TCA_ACT_FLAGS]) { + struct nla_bitfield32 *flags = RTA_DATA(tb[TCA_ACT_FLAGS]); + + if (flags->selector & TCA_ACT_FLAGS_NO_PERCPU_STATS) + print_bool(PRINT_ANY, "no_percpu", "\tno_percpu", + flags->value & + TCA_ACT_FLAGS_NO_PERCPU_STATS); + if (flags->selector & TCA_ACT_FLAGS_SKIP_HW) { + print_bool(PRINT_ANY, "skip_hw", "\tskip_hw", + flags->value & + TCA_ACT_FLAGS_SKIP_HW); + skip_hw = !!(flags->value & TCA_ACT_FLAGS_SKIP_HW); + } + if (flags->selector & TCA_ACT_FLAGS_SKIP_SW) + print_bool(PRINT_ANY, "skip_sw", "\tskip_sw", + flags->value & + TCA_ACT_FLAGS_SKIP_SW); + } + if (tb[TCA_ACT_IN_HW_COUNT] && !skip_hw) { + __u32 count = rta_getattr_u32(tb[TCA_ACT_IN_HW_COUNT]); + if (count) { + print_bool(PRINT_ANY, "in_hw", "\tin_hw", + true); + print_uint(PRINT_ANY, "in_hw_count", + " in_hw_count %u", count); + } else { + print_bool(PRINT_ANY, "not_in_hw", + "\tnot_in_hw", true); + } + } - if (flags->selector & TCA_ACT_FLAGS_NO_PERCPU_STATS) - print_bool(PRINT_ANY, "no_percpu", "\tno_percpu", - flags->value & - TCA_ACT_FLAGS_NO_PERCPU_STATS); print_nl(); } if (tb[TCA_ACT_HW_STATS])
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> --- man/man8/tc-actions.8 | 24 ++++++++++++++++++++ tc/m_action.c | 63 +++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 77 insertions(+), 10 deletions(-)