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

[Simon Horman <simon.horman@corigine.com>]

On Thu, Jul 22, 2021 at 09:33:09AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
> > On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@corigine.com> wrote:
> > > From: Baowen Zheng <baowen.zheng@corigine.com>
> > > 
> > > Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> > > offload tc action.
> > > 
> > > We offload the tc action mainly for ovs meter configuration.
> > > Make some basic changes for different vendors to return EOPNOTSUPP.
> > > 
> > > We need to call tc_cleanup_flow_action to clean up tc action entry since
> > > in tc_setup_action, some actions may hold dev refcnt, especially the mirror
> > > action.
> > > 
> > > As per review from the RFC, the kernel test robot will fail to run, so
> > > we add CONFIG_NET_CLS_ACT control for the action offload.
> > > 
> > > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> > > Signed-off-by: Louis Peens <louis.peens@corigine.com>
> > > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > > ---
> > >   drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  2 +-
> > >   .../ethernet/mellanox/mlx5/core/en/rep/tc.c   |  3 ++
> 
> > >   			    void *data,
> > >   			    void (*cleanup)(struct flow_block_cb *block_cb))
> > >   {
> > > +	if (!netdev)
> > > +		return -EOPNOTSUPP;
> > > +
> > >   	switch (type) {
> > >   	case TC_SETUP_BLOCK:
> > >   		return mlx5e_rep_indr_setup_block(netdev, sch, cb_priv, type_data,
> > > diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> 
> [..]
> 
> > > +	/* offload actions to hardware if possible */
> > > +	tcf_action_offload_cmd(actions, extack);
> > > +
> > 
> > I think this has already been suggested for RFC, but some sort of
> > visibility for offload status of action would be extremely welcome.
> > Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
> > filters.
> > 
> 
> Also showing a tc command line in the cover letter on how one would
> ask for a specific action to be offloaded.

In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.

Is it that:

* The driver (and potentially hardware, though not in our current
  implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?

With regards to a counter, I'm not quite sure what this would be:

* The number of devices where the action has been offloaded (which ties
  into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else

Regarding a flag to control offload:

* For classifiers (at least the flower classifier) there is the skip_sw and
  skip_hw flags, which allow control of placement of a classifier in SW and
  HW.
* We could add similar flags for actions, which at least in my
  world view would have the net-effect of controlling which classifiers can
  be added to sw and hw - f.e. a classifier that uses an action marked
  skip_hw could not be added to HW.
* Doing so would add some extra complexity and its not immediately apparent
  to me what the use-case would be given that there are already flags for
  classifiers.