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

[Ori Kam <orika@mellanox.com>]

Hi Nelio,

Please see my comments in line.

Ori

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Thursday, August 24, 2017 9:54 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: adrien.mazaruil@6wind.com; dev@dpdk.org
> Subject: Re: [RFC] net/mlx5: support count flow action
> 
> 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.
> 
Sorry won't happen again.

> 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?
> 
I was under the impression that main function of the RFC should also be in the message body.

> > 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.
> 
Those two lines are part of the commit message, any way I will move them.

> > 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.
> 
I haven't added them since this feature is not supported yet, and 
I don't want anybody trying to activate them.
When the feature will be supported on the verbs then I will update
those files. 

> >  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.
> 
Will be fixed
> > +			}
> > +#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.
> 
Will be fixed.

> > +#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.
> 
Will be update according to your suggestion.

> Thanks,
> 
> --
> Nélio Laranjeiro
> 6WIND
Thanks,
Ori Kam