[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b375d36a-a36c-1ac5-147d-40449987d14c@mojatatu.com>
Date: Wed, 3 Nov 2021 10:05:00 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Baowen Zheng <baowen.zheng@...igine.com>,
Vlad Buslov <vladbu@...dia.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Roi Dayan <roid@...dia.com>, Ido Schimmel <idosch@...dia.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>,
Baowen Zheng <notifications@...hub.com>,
Louis Peens <louis.peens@...igine.com>,
oss-drivers <oss-drivers@...igine.com>,
Oz Shlomo <ozsh@...dia.com>
Subject: Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of
filter and actions
On 2021-11-03 09:38, Simon Horman wrote:
> On Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
>> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>>>
>>>>> [..]
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>>>
>>>>>>> I think these flags add complexity that would be nice to avoid.
>>>>>>> But if Jamal thinks its necessary, then including the flags
>>>>>>> implementation is fine by me.
>>>>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>>>>> flag for user to specify the action should not run in software?
>>>>>>
>>>>>
>>>>> Just catching up with discussion...
>>>>> IMO, we need the flag. Oz indicated with requirement to be able
>>>>> to identify
>>>>> the action with an index. So if a specific action is added for
>>>>> skip_sw (as
>>>>> standalone or alongside a filter) then it cant be used for
>>>>> skip_hw. To illustrate
>>>>> using extended example:
>>>>>
>>>>> #filter 1, skip_sw
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>> skip_sw ip_proto tcp action police blah index 10
>>>>>
>>>>> #filter 2, skip_hw
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>> skip_hw ip_proto udp action police index 10
>>>>>
>>>>> Filter2 should be illegal.
>>>>> And when i dump the actions as so:
>>>>> tc actions ls action police
>>>>>
>>>>> For debugability, I should see index 10 clearly marked with the
>>>>> flag as skip_sw
>>>>>
>>>>> The other example i gave earlier which showed the sharing of actions:
>>>>>
>>>>> #add a policer action and offload it
>>>>> tc actions add action police skip_sw rate ... index 20 #now add
>>>>> filter1 which is
>>>>> offloaded using offloaded policer 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 and filter 1 and 2 are sharing policer instance with index 20.
>>>>>
>>>>> #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.
>>>> I think the use cases you mentioned above are clear for us. For the case:
>>>>
>>>> #add a policer action and offload it
>>>> tc actions add action police skip_sw rate ... index 20
>>>> #Now add a filter4 which has no flag
>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>> ip_proto icmp action police index 20
>>>>
>>>> Is filter4 legal?
>>>
>>> Yes it is _based on current semantics_.
>>> The reason is when adding a filter and specifying neither
>>> skip_sw nor skip_hw it defaults to allowing both.
>>> i.e is the same as skip_sw|skip_hw. You will need to have
>>> counters for both s/w and h/w (which i think is taken care of today).
>>>
>>>
>>
>> Apologies, i will like to take this one back. Couldnt stop thinking
>> about it while sipping coffee;->
>> To be safe that should be illegal. The flags have to match _exactly_
>> for both action and filter to make any sense. i.e in the above case
>> they are not.
>
> I could be wrong, but I would have thought that in this case the flow
> is legal but is only added to hw (because the action doesn't exist in sw).
>
I was worried what would show up in a dump of the filter.
Would it show only the h/w counter? And if yes, is the s/w
version mutated with no policer (since the policer is only
in h/w)?
> But if you prefer to make it illegal I guess that is ok too.
It just seemed easier from manageability pov to make it illegal, no?
i.e if flags dont match exactly it is illegal is a simple check.
cheers,
jamal
Powered by blists - more mailing lists