Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add group counter support to rte_flow

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Fri, Apr 06, 2018 at 01:24:00PM +0100, Declan Doherty wrote:
> Add new RTE_FLOW_ACTION_TYPE_GROUP_COUNT action type to enable shared
> counters across multiple flows on a single port or across multiple
> flows on multiple ports within the same switch domain.
> 
> Introduce new API rte_flow_query_group_count to allow querying of group
> counters.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

Both features are definitely needed, however I suggest to enhance the
existing action type and query function instead, given the rte_flow ABI
won't be maintained for the 18.05 release [1].

Counters and query support were defined as a kind of PoC in preparation for
future requirements back in DPDK 17.02 and so far few PMDs have implemented
the query callback (mlx5 and failsafe, and the latter isn't really a PMD).

Due to the behavior change of action lists [2], providing an action type as
a query parameter is not specific enough anymore, for instance if a list
contains multiple COUNT, the application should be able to tell which needs
to be queried.

Therefore I suggest to redefine the query function as follows:

 int
 rte_flow_query(uint16_t port_id,
                struct rte_flow *flow,
                const struct rte_flow_action *action,
                void *data,
                struct rte_flow_error *error);

Third argument is an action definition with the same configuration (if any)
as previously defined in the action list originally used to create the flow
rule (not necessarily the same pointer, only the contents matter).

It means two perfectly identical actions can't be distinguished, and that's
how group counters will work.

Instead of adding a new action type to distinguish groups, a configuration
structure is added to the existing RTE_FLOW_ACTION_TYPE_COUNT, with
non-shared counters as a default behavior:

 struct rte_flow_action_count {
         uint32_t shared:1; /**< Share counter ID with other flow rules. */
         uint32_t reserved:31; /**< Reserved, must be zero. */
         uint32_t id; /**< Counter ID. */
 };

Doing so will impact some existing code in mlx5 and librte_flow_classify,
but that shouldn't be much of an issue.

Keep in mind testpmd and its documentation must be updated as well.

Thoughts?

A few nits below for the sake of commenting.

[1] "Flow API overhaul for switch offloads"
    http://dpdk.org/ml/archives/dev/2018-April/095774.html
[2] "ethdev: alter behavior of flow API actions"
    http://dpdk.org/ml/archives/dev/2018-April/095779.html

> ---
>  doc/guides/prog_guide/rte_flow.rst      | 35 +++++++++++++++++++++
>  lib/librte_ether/rte_ethdev_version.map |  8 +++++
>  lib/librte_ether/rte_flow.c             | 21 +++++++++++++
>  lib/librte_ether/rte_flow.h             | 56 ++++++++++++++++++++++++++++++++-
>  lib/librte_ether/rte_flow_driver.h      |  6 ++++
>  5 files changed, 125 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 961943d..fd33d19 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1698,6 +1698,41 @@ Return values:
>  
>  - 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
>  
> +

Unnecessary empty line.

> +Group Count Query
> +~~~~~~~~~~~~~~~~~
> +
> +Query group counter which can be associated with multiple flows on a specified
> +port.
> +
> +This function allows retrieving of group counters. A group counter is a
> +counter which can be shared among multiple flows on a single port or among
> +multiple flows on multiple ports within the same switch domain. Data is
> +gathered by special actions which must be present in the flow rule
> +definition.
> +
> +.. code-block:: c
> +
> +   int
> +   rte_flow_query_group_count(uint16_t port_id,
> +			   uint32_t group_counter_id,
> +			   struct rte_flow_query_count *count,
> +               struct rte_flow_error *error);
> +
> +Arguments:
> +
> +- ``port_id``: port identifier of Ethernet device.
> +- ``group_counter_id``: group counter identifier.
> +- ``count``: group counter parameters.
> +- ``error``: perform verbose error reporting if not NULL. PMDs initialize
> +  this structure in case of error only.
> +
> +Return values:
> +
> +- 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
> +
> +
> +

More unnecessary empty lines.

>  Isolated mode
>  -------------
>  
> diff --git a/lib/librte_ether/rte_ethdev_version.map b/lib/librte_ether/rte_ethdev_version.map
> index 34df6c8..cff6807 100644
> --- a/lib/librte_ether/rte_ethdev_version.map
> +++ b/lib/librte_ether/rte_ethdev_version.map
> @@ -229,3 +229,11 @@ EXPERIMENTAL {
>  	rte_mtr_stats_update;
>  
>  } DPDK_17.11;
> +
> +

One more.

> +EXPERIMENTAL {
> +	global:
> +
> +	rte_flow_query_group_count
> +
> +} DPDK_18.05;
> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> index 38f2d27..e10b1d0 100644
> --- a/lib/librte_ether/rte_flow.c
> +++ b/lib/librte_ether/rte_flow.c
> @@ -418,3 +418,24 @@ rte_flow_copy(struct rte_flow_desc *desc, size_t len,
>  	}
>  	return 0;
>  }
> +
> +int __rte_experimental
> +rte_flow_query_group_count(uint16_t port_id,
> +	uint32_t group_count_id,
> +	struct rte_flow_query_count *count,
> +	struct rte_flow_error *error)

This function lacks a short documentation comment (see rte_flow_query()).

> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +
> +	if (!ops)
> +		return -rte_errno;
> +	if (likely(!!ops->query_group_count))
> +		return flow_err(port_id,
> +				ops->query_group_count(dev, group_count_id,
> +						       count, error),
> +				error);
> +	return rte_flow_error_set(error, ENOSYS,
> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +				  NULL, rte_strerror(ENOSYS));
> +}
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 13e4202..7d1f89d 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1010,7 +1010,19 @@ enum rte_flow_action_type {
>  	 *
>  	 * See struct rte_flow_action_security.
>  	 */
> -	RTE_FLOW_ACTION_TYPE_SECURITY
> +	RTE_FLOW_ACTION_TYPE_SECURITY,
> +
> +	/**
> +	 * Enable a shared flow group counter for flow. Group counters can be
> +	 * associated with multiples flows on the same port or on port within
> +	 * the same switch domain if supported by that device.
> +	 *
> +	 * Group counters can be retrieved and reset through
> +	 * rte_flow_query_group_count()
> +	 *
> +	 * See struct rte_flow_action_group_count.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_GROUP_COUNT

Don't forget the trailing comma.

>  };
>  
>  /**
> @@ -1149,6 +1161,18 @@ struct rte_flow_action_security {
>  };
>  
>  /**
> + * RTE_FLOW_ACTION_TYPE_GROUP_COUNT
> + *
> + * A packet/byte counter which can be shared across a group of flows programmed
> + * on the same port/switch domain.
> + *
> + * Non-terminating by default.
> + */
> +struct rte_flow_action_group_count {
> +	uint32_t id;
> +};
> +
> +/**
>   * Definition of a single action.
>   *
>   * A list of actions is terminated by a END action.
> @@ -1476,6 +1500,36 @@ rte_flow_copy(struct rte_flow_desc *fd, size_t len,
>  	      const struct rte_flow_item *items,
>  	      const struct rte_flow_action *actions);
>  
> +

Caught another empty line.

> +/**
> + * Get hit/bytes count for group counter.
> + *
> + * A group counter is a counter which can be shared among multiple flows on a
> + * single port or among multiple flows on multiple ports within the same
> + * switch domain.
> + *
> + * In the case of ports within the same switch domain a global name space is
> + * assumed for group_count_id value.
> + *
> + * @param[in]	port_id
> + *   Port identifier of Ethernet device.
> + * @param[in]	group_count_id
> + *   Group counter identifier to query
> + * @param[out]	count
> + *   Group counter value
> + * @param[out]	error
> + *   Perform verbose error reporting if not NULL. PMDs initialize this
> + *   structure in case of error only.
> + *
> + * @return
> + *   Negative error code (errno value) and rte_errno is set.
> + */
> +int __rte_experimental
> +rte_flow_query_group_count(uint16_t port_id,
> +			   uint32_t group_count_id,
> +			   struct rte_flow_query_count *count,
> +			   struct rte_flow_error *error);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_ether/rte_flow_driver.h b/lib/librte_ether/rte_flow_driver.h
> index 7778c8e..ef09465 100644
> --- a/lib/librte_ether/rte_flow_driver.h
> +++ b/lib/librte_ether/rte_flow_driver.h
> @@ -96,6 +96,12 @@ struct rte_flow_ops {
>  		(struct rte_eth_dev *,
>  		 int,
>  		 struct rte_flow_error *);
> +	/** See rte_flow_query_group_count(). */
> +	int (*query_group_count)
> +		(struct rte_eth_dev *,
> +		 uint32_t,
> +		 struct rte_flow_query_count *,
> +		 struct rte_flow_error *);
>  };
>  
>  /**
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND