Re: [RFC/PATCH net-next v3 0/8] allow user to offload tc action to net device

On 11/2/2021 6:15 PM, Simon Horman wrote:

On Tue, Nov 02, 2021 at 05:33:14PM +0200, Vlad Buslov wrote:

quoted

On Tue 02 Nov 2021 at 14:51, Simon Horman [off-list ref] wrote:

quoted

On Mon, Nov 01, 2021 at 10:01:28AM +0200, Vlad Buslov wrote:

quoted

On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 2021-10-31 05:50, Oz Shlomo wrote:

quoted

On 10/28/2021 2:06 PM, Simon Horman wrote:

...

quoted

quoted

quoted

Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
      ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
      ip_proto tcp action mirred index 1

I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.

Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.

Hi,

I feel that I am missing some very obvious point here.

But from my perspective the use case described by Oz is supported
by existing offload of the flower classifier (since ~4.13 IIRC).

Mlx5 driver can't support such case without infrastructure change in
kernel for following reasons:

- Action index is not provided by flow_action offload infrastructure for
   most of the actions, so there is no way for driver to determine
   whether the action is shared.

- If we extend the infrastructure to always provide tcfa_index (a
   trivial change), there would be not much use for it because there is
   no way to properly update shared action counters without
   infrastructure code similar to what you implemented as part of this
   series.

How do you support shared actions created through cls_api in your
driver, considering described limitations?

Thanks,

I misread the use case described by Oz, but I believe I understand it now.

I agree that the case described is neither currently supported, nor
supported by this patchset (to be honest I for one had not considered it).

So, I think the question is: does upporting this use-case make sense - from
implementation, use-case, and consistency perspectives - in the context of
this patchset?

Am I on the right track?

Currently we don't have a specific application use case for sharing actions that were created by tc 
filters. However, we do have future use case in mind.
We could add such functionality on top of this series when a use case will materialize.
Perhaps, at that point, we can also introduce a control flag in order to avoid unnecessary insertion 
rate performance degradation.