Re: [dpdk-dev] [RFC] net/mlx5: support count flow action

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Ori,

Please keep the coding style of the file, and pass checkpatch before
submitting a patch on the mailing list.  It helps the review by having a
correct patch respecting the coding style of the file.
I won't spot out here all the coding style issues, if you need some help, feel
free to ask.

On Mon, Aug 21, 2017 at 03:35:41PM +0300, Ori Kam wrote:
> Support count flow action.

Why copy/pasting the title in the commit message?

> This patch is basic design only, do to missing features on the verbs
> driver. As soon as the features will be implemented on the verbs driver this
> will be updated and rebased on top of dpdk.org/ml/archives/dev/2017-August/072351.html
> (The verbs driver should be ready starting September)
>
> This RFC should be applied on top of
> dpdk.org/ml/archives/dev/2017-August/072351.html

Last two comments should be after '---' line.

> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.h      |   4 ++
>  drivers/net/mlx5/mlx5_flow.c | 163 ++++++++++++++++++++++++++++++++++++++++++-

There are missing changes in the Makefile to have the
HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT and the include of the
mlx5_autoconf.h in mlx5_flow.c.

>  2 files changed, 166 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index e89aba8..434e848 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
>[...] 
> +/**
> + * Query an existing flow rule.
> + *
> + * @see rte_flow_query()
> + * @see rte_flow_ops
> + */
> +int
> +mlx5_flow_query(struct rte_eth_dev *dev,
> +		struct rte_flow *flow,
> +		enum rte_flow_action_type type,
> +		void *res,
> +		struct rte_flow_error *error)
> +{
> +
> +	int res_value = 0;
> +	switch (type){
> +		case RTE_FLOW_ACTION_TYPE_COUNT:
> +			if (!flow->counter) {
> +				rte_flow_error_set(error, EINVAL,
> +								   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +								   NULL,
> +								   "No counter is set for this flow");
> +				return -1;

Wrong returned value, read the rte_flow_query API allowed values.

> +			}
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
> +			res_value = priv_flow_query_counter(mlx5_get_priv(dev), flow->counter,
> +					(struct rte_flow_query_count*)res,
> +					error);
> +#else
> +			rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
> +									NULL, "Flow count unsupported");
> +			(void)dev;
> +			(void)flow;
> +			(void)type;
> +			(void)res;
> +			(void)error;
> +			return -1;

Same here.

> +#endif

I'll suggest to have a dedicated function here to handle this situation, like
a mlx5_flow_query_counters() and call it from this case.  It will clearly ease
the readability and maintenance.

Thanks,

-- 
Nélio Laranjeiro
6WIND