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

On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:

On 2021-07-28 10:46 a.m., Simon Horman wrote:

quoted

On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:

quoted

On 2021-07-28 3:46 a.m., Simon Horman wrote:

quoted

On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:

quoted

On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 2021-07-27 10:38 a.m., Vlad Buslov wrote:

quoted

On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:

[..]

quoted

quoted

quoted

quoted

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'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.

It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?

How about we confirm that once we've implemented the feature.

But I would assume that:

* Existing methods for adding rules work as before
* When one dumps an action (in a sufficiently verbose
  way) the in_hw and in_hw_counter fields are displayed as they are for
  filters.

Does that help?

quoted

If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.

My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
We have spent literally hours debugging issues where rules are being
offloaded thinking it was the driver so any extra info helps.

I do think there is a value to symmetry between the APIs.
And I don't think doing so moves things in a bad direction.
But rather a separate discussion is needed to discuss how to
improve debuggability.

...

quoted

quoted

quoted

quoted

quoted

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

?

quoted

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.

You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.

Then we are good. These are the same patches you have here?

Yes.