Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute

[Ori Kam <orika@nvidia.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
> 
> Hi Ori,
> 
> On 10/11/21 1:02 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> > Sorry but I think I missed something.
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> >> Sent: Friday, October 8, 2021 1:26 PM
> >>
> >> Indirect actions should be used to do shared counters.
> >>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> >> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> >> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com>
> >> Acked-by: Ori Kam <orika@nvidia.com>
> >> ---
> >> v2:
> >>     - remove reserved field from count structure (review from Stephen)
> >>     - apply mlx5 review notes from Matan
> >>
> >>  app/test-pmd/cmdline_flow.c                |  10 --
> >>  doc/guides/prog_guide/rte_flow.rst         |  19 +---
> >>  doc/guides/rel_notes/deprecation.rst       |   4 -
> >>  doc/guides/rel_notes/release_21_11.rst     |   4 +
> >>  drivers/net/bnxt/tf_ulp/ulp_rte_parser.c   |   5 -
> >>  drivers/net/cnxk/cnxk_rte_flow.c           |   8 --
> >>  drivers/net/hns3/hns3_flow.c               |   3 +-
> >>  drivers/net/ice/ice_fdir_filter.c          |   4 +-
> >>  drivers/net/mlx5/mlx5.c                    |  11 --
> >>  drivers/net/mlx5/mlx5.h                    |   9 --
> >>  drivers/net/mlx5/mlx5_flow_dv.c            | 118 ++-------------------
> >>  drivers/net/mlx5/mlx5_flow_verbs.c         |  22 +---
> >>  drivers/net/octeontx2/otx2_flow_parse.c    |  10 --
> >>  drivers/net/sfc/sfc_mae.c                  |   9 +-
> >>  drivers/net/softnic/rte_eth_softnic_flow.c |   7 --
> >>  lib/ethdev/rte_flow.h                      |  16 +--
> >>  16 files changed, 22 insertions(+), 237 deletions(-)
> >>
> >
> > [Snip]
> >
> >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> >> 7b1ed7f110..9819c25d2f 100644
> >> --- a/lib/ethdev/rte_flow.h
> >> +++ b/lib/ethdev/rte_flow.h
> >> @@ -75,7 +75,7 @@ extern "C" {
> >>   * At least one direction must be specified.
> >>   *
> >>   * Specifying both directions at once for a given rule is not
> >> recommended
> >> - * but may be valid in a few cases (e.g. shared counter).
> >> + * but may be valid in a few cases.
> >>   */
> >>  struct rte_flow_attr {
> >>  	uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@
> >> struct rte_flow_query_age {
> >>   * Counters can be retrieved and reset through ``rte_flow_query()``, see
> >>   * ``struct rte_flow_query_count``.
> >>   *
> >> - * @deprecated Shared attribute is deprecated, use generic
> >> - * RTE_FLOW_ACTION_TYPE_INDIRECT action.
> >> - *
> >> - * The shared flag indicates whether the counter is unique to the
> >> flow rule the
> >> - * action is specified with, or whether it is a shared counter.
> >> - *
> >> - * For a count action with the shared flag set, then then a global
> >> device
> >> - * namespace is assumed for the counter id, so that any matched flow
> >> rules using
> >> - * a count action with the same counter id on the same port will
> >> contribute to
> >> - * that counter.
> >> - *
> >>   * For ports within the same switch domain then the counter id namespace extends
> >>   * to all ports within that switch domain.
> >
> > I don't think we need this anymore.
> 
> I agree. I'll remove it in v3 if required, but I hope it could be removed on apply as well.
> 
> >
> >>   */
> >>  struct rte_flow_action_count {
> >> -	/** @deprecated Share counter ID with other flow rules. */
> >> -	uint32_t shared:1;
> >> -	uint32_t reserved:31; /**< Reserved, must be zero. */
> >>  	uint32_t id; /**< Counter ID. */
> >
> > Why do we need to keep the id field?
> 
> It is a very good question. I thought about it and preserved it for the corner case of two COUNT actions in
> the same rule.
> If so, id is required to distinguish on query.
> I don't know if we really need it to have two basically duplicate counters in the same rule. However, since
> order of actions matter, COUNT, VXLAN_ENCAP, COUNT should produce different byte counters.
> 
Good thinking, I will not block this patch. 
Just please fix the comment, if it can be done in apply that will be good for me

> I suggest to continue discussion and gather more thought on it, but do not block the patch, since strictly
> speaking it is a bit separate topic as noted above.
> 
Yes lets take it in one of the RTE_FLOW meetings or over mail.

> Thanks,
> Andrew.

Best,
Ori