From: Vlad Buslov <vladbu@nvidia•com>
To: Baowen Zheng <baowen.zheng@corigine•com>
Cc: Jamal Hadi Salim <jhs@mojatatu•com>,
Simon Horman <simon.horman@corigine•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
Roi Dayan <roid@nvidia•com>, Ido Schimmel <idosch@nvidia•com>,
Cong Wang <xiyou.wangcong@gmail•com>,
Jiri Pirko <jiri@resnulli•us>,
Baowen Zheng <notifications@github•com>,
Louis Peens <louis.peens@corigine•com>,
oss-drivers <oss-drivers@corigine•com>
Subject: Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions
Date: Mon, 1 Nov 2021 09:38:34 +0200 [thread overview]
Message-ID: <ygnhcznk9vgl.fsf@nvidia.com> (raw)
In-Reply-To: <DM5PR1301MB21728931E03CFE4FA45C5DD3E78A9@DM5PR1301MB2172.namprd13.prod.outlook.com>
On Mon 01 Nov 2021 at 05:29, Baowen Zheng <baowen.zheng@corigine•com> wrote:
> On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
>>On 2021-10-30 22:27, Baowen Zheng wrote:
>>> Thanks for your review, after some considerarion, I think I understand what
>>you are meaning.
>>>
>>
>>[..]
>>
>>>>>> I know Jamal suggested to have skip_sw for actions, but it
>>>>>> complicates the code and I'm still not entirely understand why it is
>>necessary.
>>>>>
>>>>> If the hardware can independently accept an action offload then
>>>>> skip_sw per action makes total sense. BTW, my understanding is
>>>>
>>>> Example configuration that seems bizarre to me is when offloaded
>>>> shared action has skip_sw flag set but filter doesn't. Then behavior
>>>> of classifier that points to such action diverges between hardware
>>>> and software (different lists of actions are applied). We always try
>>>> to make offloaded TC data path behave exactly the same as software
>>>> and, even though here it would be explicit and deliberate, I don't
>>>> see any practical use-case for this.
>>> We add the skip_sw to keep compatible with the filter flags and give
>>> the user an option to specify if the action should run in software. I
>>> understand what you mean, maybe our example is not proper, we need to
>>> prevent the filter to run in software if the actions it applies is skip_sw, so we
>>need to add more validation to check about this.
>>> Also I think your suggestion makes full sense if there is no use case
>>> to specify the action should not run in sw and indeed it will make our
>>> implement more simple if we omit the skip_sw option.
>>> Jamal, WDYT?
>>
>>
>>Let me use an example to illustrate my concern:
>>
>>#add a policer offload it
>>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>>offloaded 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 so far...
>>#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.
>>
>>If we had added the policer without skip_sw and without skip_hw then i think
>>filter3 should have been legal (we just need to account for stats in_hw vs
>>in_sw).
>>
>>Not sure if that makes sense (and addresses Vlad's earlier comment).
>>
> I think the cases you mentioned make sense to us. But what Vlad concerns is the use
> case as:
> #add a policer offload it
> tc actions add action police skip_sw rate ... index 20
> #now add filter4 which can't be offloaded
> tc filter add dev $DEV1 proto ip parent ffff: flower \
> ip_proto tcp action police index 20
> it is possible the filter4 can't be offloaded, then filter4 will run in software,
> should this be legal?
> Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
> will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
> an action? If a packet matches the filter in software, the action should not be skip_sw.
> If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work.
> Of course, we can also keep skip_sw by adding more check to avoid the above case.
>
> Vlad, I am not sure if I understand your idea correctly.
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.
next prev parent reply other threads:[~2021-11-01 7:38 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-28 11:06 [RFC/PATCH net-next v3 0/8] allow user to offload tc action to net device Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 1/8] flow_offload: fill flags to action structure Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 2/8] flow_offload: reject to offload tc actions in offload drivers Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 3/8] flow_offload: allow user to offload tc action to net device Simon Horman
2021-10-29 16:59 ` Vlad Buslov
2021-11-01 9:44 ` Baowen Zheng
2021-11-01 12:05 ` Vlad Buslov
2021-11-02 1:38 ` Baowen Zheng
2021-10-31 9:50 ` Oz Shlomo
2021-11-01 2:30 ` Baowen Zheng
2021-11-01 10:07 ` Oz Shlomo
2021-11-01 10:27 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 4/8] flow_offload: add skip_hw and skip_sw to control if offload the action Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 5/8] flow_offload: add process to update action stats from hardware Simon Horman
2021-10-29 17:11 ` Vlad Buslov
2021-11-01 10:07 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 6/8] net: sched: save full flags for tc action Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 7/8] flow_offload: add reoffload process to update hw_count Simon Horman
2021-10-29 17:31 ` Vlad Buslov
2021-11-02 9:20 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions Simon Horman
2021-10-29 18:01 ` Vlad Buslov
2021-10-30 10:54 ` Jamal Hadi Salim
2021-10-30 14:45 ` Vlad Buslov
[not found] ` <DM5PR1301MB21722A85B19EE97EFE27A5BBE7899@DM5PR1301MB2172.namprd13.prod.outlook.com>
2021-10-31 13:30 ` Jamal Hadi Salim
2021-11-01 3:29 ` Baowen Zheng
2021-11-01 7:38 ` Vlad Buslov [this message]
2021-11-02 12:39 ` Simon Horman
2021-11-03 7:57 ` Baowen Zheng
2021-11-03 10:13 ` Jamal Hadi Salim
2021-11-03 11:30 ` Baowen Zheng
2021-11-03 12:33 ` Jamal Hadi Salim
2021-11-03 13:33 ` Jamal Hadi Salim
2021-11-03 13:38 ` Simon Horman
2021-11-03 14:05 ` Jamal Hadi Salim
2021-11-03 14:03 ` Baowen Zheng
2021-11-03 14:16 ` Jamal Hadi Salim
2021-11-03 14:48 ` Baowen Zheng
2021-11-03 15:35 ` Jamal Hadi Salim
2021-11-03 13:37 ` Baowen Zheng
2021-11-04 2:30 ` Baowen Zheng
2021-11-04 5:51 ` Baowen Zheng
2021-11-04 9:07 ` Vlad Buslov
2021-11-04 11:15 ` Baowen Zheng
2021-10-28 14:23 ` [RFC/PATCH net-next v3 0/8] allow user to offload tc action to net device Jamal Hadi Salim
2021-10-28 14:39 ` Jamal Hadi Salim
2021-10-31 9:50 ` Oz Shlomo
2021-10-31 12:03 ` Dave Taht
2021-10-31 14:14 ` Jamal Hadi Salim
2021-10-31 14:19 ` Jamal Hadi Salim
2021-11-01 14:27 ` Dave Taht
2021-10-31 13:40 ` Jamal Hadi Salim
2021-11-01 8:01 ` Vlad Buslov
2021-11-02 12:51 ` Simon Horman
2021-11-02 15:33 ` Vlad Buslov
2021-11-02 16:15 ` Simon Horman
2021-11-03 10:56 ` Oz Shlomo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ygnhcznk9vgl.fsf@nvidia.com \
--to=vladbu@nvidia$(echo .)com \
--cc=baowen.zheng@corigine$(echo .)com \
--cc=idosch@nvidia$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=louis.peens@corigine$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=notifications@github$(echo .)com \
--cc=oss-drivers@corigine$(echo .)com \
--cc=roid@nvidia$(echo .)com \
--cc=simon.horman@corigine$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox