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

From: Simon Horman <[email-masked]>
Date: Wed, 28 Jul 2021 16:46:26 +0200

On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
> 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 <[email-masked]> wrote:
> > > > On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
> > > > > On Tue 27 Jul 2021 at 16:04, Simon Horman <[email-masked]> 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'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.

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

> [..]
> 
> > 
> > > > 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.

There is a per-netdev (not global as I wrote above) flag to enable and
disable offload. And there is per-classifier skip_hw flag. I can dig
through the code as easily as you can but I'd be surprised if the
driver is seeing offload requests if either of those settings
are in effect.

> > > > 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.

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.