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

Message ID 1523017443-12414-2-git-send-email-declan.doherty@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply issues

Commit Message

Doherty, Declan April 6, 2018, 12:24 p.m. UTC
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>
---
 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(-)
  

Comments

Adrien Mazarguil April 6, 2018, 8:26 p.m. UTC | #1
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
>
  
Mohammad Abdul Awal April 9, 2018, 2:22 p.m. UTC | #2
Hi Adrien,


On 06/04/2018 21:26, Adrien Mazarguil wrote:
> 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?
Please correct me if I am wrong but I think we are talking two different 
things here.
If I understood you correctly, you are proposing to pass a list of 
actions (instead if a action type) in the third parameter to perform 
multiple actions in the same query call. Lets take an example for 100 
ingress flows. So, if we want to query the counter for all the flows, we 
can get them by a single query providing a list (of size 100) of 
action_count in the 3rd param.

On the other hand, we are saying that all the flows are belongs to same 
tunnel end-point (we are talking only 1 TEP here), then the PMD will be 
responsible to increment the counter of TEP for matching all the flows 
(100 flows). So, using one group query by passing one action_count in 
3rd param, we can get the count of the TEP.

This case is generic enough for sure for simple flows but may not be 
suitable for tunnel cases, as application needs to track the counters 
for all the flows, and needs to build the list of action each time the 
flows added/deleted.

>
> 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.
I will take into account all the comments.

Regards,
Awal.

>
>> +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 April 9, 2018, 3:23 p.m. UTC | #3
Hi Mohammad,

On Mon, Apr 09, 2018 at 03:22:45PM +0100, Mohammad Abdul Awal wrote:
> Hi Adrien,
> 
> 
> On 06/04/2018 21:26, Adrien Mazarguil wrote:
> > 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?
> Please correct me if I am wrong but I think we are talking two different
> things here.
> If I understood you correctly, you are proposing to pass a list of actions
> (instead if a action type) in the third parameter to perform multiple
> actions in the same query call. Lets take an example for 100 ingress flows.
> So, if we want to query the counter for all the flows, we can get them by a
> single query providing a list (of size 100) of action_count in the 3rd
> param.

Whoa no! I'm only suggesting a pointer to a single action as a replacement
for the basic action type, not a *list* of actions. I hope this addresses
all concerns :)

The fact the action in question would refer to a shared counter (see struct
above) with a given ID would be enough to make all counters with the same ID
refer to the same internal PMD object.

> On the other hand, we are saying that all the flows are belongs to same
> tunnel end-point (we are talking only 1 TEP here), then the PMD will be
> responsible to increment the counter of TEP for matching all the flows (100
> flows). So, using one group query by passing one action_count in 3rd param,
> we can get the count of the TEP.
> 
> This case is generic enough for sure for simple flows but may not be
> suitable for tunnel cases, as application needs to track the counters for
> all the flows, and needs to build the list of action each time the flows
> added/deleted.

I think we're on the same page. I'm only suggesting to define a
configuration structure for the COUNT action (which currently doesn't take
any) as a replacement for GROUP_COUNT, and tweak the query callback to be
able to tell a specific counter to query instead of adding a new query
callback and leaving the existing one broken by design.

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

Patch

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.
 
+
+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.
+
+
+
 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;
+
+
+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)
+{
+	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
 };
 
 /**
@@ -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);
 
+
+/**
+ * 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 *);
 };
 
 /**