Hi Nelio, Please see my comments in line.
Ori > -----Original Message----- > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com] > Sent: Thursday, August 24, 2017 9:54 AM > To: Ori Kam <or...@mellanox.com> > Cc: adrien.mazar...@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 <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. > 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
