Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 2021-07-28 3:46 a.m., Simon Horman wrote:
> On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
>> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>>>> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:

[..]

>>>> I think we have the same issue with filters - they might not be in
>>>> hardware after driver callback returned "success" (due to neigh state
>>>> being invalid for tunnel_key encap, for example).
>>>>
>>>
>>> Sounds like we need another state for this. Otherwise, how do you debug
>>> that something is sitting in the driver and not in hardware after you
>>> issued a command to offload it? How do i tell today?
>>> Also knowing reason why something is sitting in the driver would be
>>> helpful.
>>
>> It is not about just adding another state. The issue is that there is no
>> way for drivers to change the state of software filter dynamically.
> 
> I think it might be worth considering enhancing things at some point.
> But I agree that its more than a matter of adding an extra flag. And
> I think it's reasonable to implement something similar to the classifier
> current offload handling of IN_HW now and consider enhancements separately.
> 

Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.

So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa

And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.

[..]

> 
>>> I didnt follow this:
>>> Are we refering to the the "block" semantics (where a filter for
>>> example applies to multiple devices)?
>>
>> This uses indirect offload infrastructure, which means all drivers
>> in flow_block_indr_dev_list will receive action offload requests.
>>

Ok, understood.

[..]

>>
>> No. How would adding more flags improve h/w update rate? I was just
>> thinking that it is strange that users that are not interested in
>> offloads would suddenly have higher memory usage for their actions just
>> because they happen to have offload-capable driver loaded. But it is not
>> a major concern for me.
> 
> In that case can we rely on the global tc-offload on/off flag
> provided by ethtool? (I understand its not the same, but perhaps
> it is sufficient in practice.)
> 

ok.
So: I think i have seen this what is probably the spamming refered
with the intel (800?) driver ;-> Basically driver was reacting to
all filters regardless of need to offload or not.
I thought it was an oversight on their part and the driver needed
fixing. Are we invoking the offload regardless of whether h/w offload
is requested? In my naive view - at least when i looked at the intel
code - it didnt seem hard to avoid the spamming.


>>> I was looking at it more as a (currently missing) feature improvement.
>>> We already have a use case that is implemented by s/w today. The feature
>>> mimics it in h/w.
>>>
>>> At minimal all existing NICs should be able to support the counters
>>> as mapped to simple actions like drop. I understand for example if some
>>> cant support adding separately offloading of tunnels for example.
>>> So the syntax is something along the lines of:
>>>
>>> tc actions add action drop index 15 skip_sw
>>> tc filter add dev ...parent ... protocol ip prio X ..\
>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>>>
>>> You get an error if counter index 15 is not offloaded or
>>> if skip_sw was left out..
>>>
>>> And then later on, if you support sharing of actions:
>>> tc filter add dev ...parent ... protocol ip prio X2 ..\
>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
> 
> Right, I understand that makes sense and is internally consistent.
> But I think that in practice it only makes a difference "Approach B"
> implementations, none of which currently exist.
> 

At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15

?

> I would suggest we can add this when the need arises, rather than
> speculatively without hw/driver support. Its not precluded by the current
> model AFAIK.
> 

We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.

cheers,
jamal