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 <or...@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
