[dpdk-dev,RFC] net/mlx5: support count flow action
Checks
Commit Message
Support count flow action.
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
Signed-off-by: Ori Kam <orika@mellanox.com>
---
drivers/net/mlx5/mlx5.h | 4 ++
drivers/net/mlx5/mlx5_flow.c | 163 ++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 166 insertions(+), 1 deletion(-)
Comments
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,
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
Hi Ori,
On Thu, Aug 24, 2017 at 02:04:32PM +0000, Ori Kam wrote:
> 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.
No problem, first contribution is always complicate.
> > 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.
No, it is not necessary, the commit message should bring useful information by
still being short and precise.
>[...]
> > > ---
> > > 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.
Ok, so a new version should be sent soon :)
>[...]
> >
> Will be update according to your suggestion.
Thanks,
@@ -294,6 +294,10 @@ struct rte_flow *mlx5_flow_create(struct rte_eth_dev *,
int mlx5_flow_destroy(struct rte_eth_dev *, struct rte_flow *,
struct rte_flow_error *);
int mlx5_flow_flush(struct rte_eth_dev *, struct rte_flow_error *);
+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 mlx5_flow_isolate(struct rte_eth_dev *, int, struct rte_flow_error *);
int priv_flow_start(struct priv *);
void priv_flow_stop(struct priv *);
@@ -98,7 +98,9 @@ struct rte_flow {
uint16_t rxqs_n; /**< Number of queues in this flow, 0 if drop queue. */
uint32_t mark:1; /**< Set if the flow is marked. */
uint32_t drop:1; /**< Drop queue. */
+ uint32_t count:1; /**< Count action. */
uint64_t hash_fields; /**< Fields that participate in the hash. */
+ struct ibv_counter_set *counter; /**< Pointer to the counter struct. */
struct rxq *rxqs[]; /**< Pointer to the queues array. */
};
@@ -150,6 +152,7 @@ struct mlx5_flow_items {
RTE_FLOW_ACTION_TYPE_QUEUE,
RTE_FLOW_ACTION_TYPE_MARK,
RTE_FLOW_ACTION_TYPE_FLAG,
+ RTE_FLOW_ACTION_TYPE_COUNT,
RTE_FLOW_ACTION_TYPE_END,
};
@@ -291,6 +294,7 @@ struct mlx5_flow_action {
uint32_t queue:1; /**< Target is a receive queue. */
uint32_t drop:1; /**< Target is a drop queue. */
uint32_t mark:1; /**< Mark is present in the flow. */
+ uint32_t count:1; /**< Count is present in the flow. */
uint32_t mark_id; /**< Mark identifier. */
uint16_t queues[RTE_MAX_QUEUES_PER_PORT]; /**< Queues indexes to use. */
uint16_t queues_n; /**< Number of entries in queue[]. */
@@ -567,6 +571,8 @@ struct mlx5_flow_action {
action->mark_id = mark->id;
} else if (actions->type == RTE_FLOW_ACTION_TYPE_FLAG) {
action->mark = 1;
+ } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) {
+ action->count = 1;
} else {
goto exit_action_not_supported;
}
@@ -575,7 +581,13 @@ struct mlx5_flow_action {
flow->offset += sizeof(struct ibv_exp_flow_spec_action_tag);
if (!flow->ibv_attr && action->drop)
flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
- if (!action->queue && !action->drop) {
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
+ if (action->count && !flow->ibv_attr)
+ flow->offset += sizeof(struct ibv_exp_flow_spec_action_count);
+#else
+ goto exit_action_not_supported;
+#endif
+ if (!action->queue && !action->drop && !action->count) {
rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_HANDLE,
NULL, "no valid action");
return -rte_errno;
@@ -611,6 +623,7 @@ struct mlx5_flow_action {
.queue = 0,
.drop = 0,
.mark = 0,
+ .count = 0,
.mark_id = MLX5_FLOW_MARK_DEFAULT,
.queues_n = 0,
};
@@ -1142,6 +1155,44 @@ struct mlx5_flow_action {
return NULL;
}
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
+/**
+ * Connect the counter and add the action to the flow.
+ *
+ * @param priv
+ * Pointer to private structure.
+ * @param flow
+ * Pointer to flow structure.
+ * @param counter_id
+ * Counter id.
+ *
+ * @return
+ * Pointer to counter set.
+ */
+static struct ibv_counter_set *
+priv_flow_create_counter(struct priv *priv, struct mlx5_flow *flow, uint16_t counter_id)
+{
+
+ struct ibv_exp_flow_spec_action_count *count;
+ unsigned int size = sizeof(struct ibv_exp_flow_spec_action_count);
+ struct ibv_counter_set *counter;
+
+ counter = ibv_create_counter_set(priv->ctx,0);
+ if (!counter) {
+ return NULL;
+ }
+
+ count = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
+ *count = (struct ibv_exp_flow_spec_action_tag){
+ .type = IBV_EXP_FLOW_SPEC_ACTION_COUNT,
+ .size = size,
+ .counter_set = counter,
+ };
+ ++flow->ibv_attr->num_of_specs;
+ return counter;
+}
+#endif
+
/**
* Convert a flow.
*
@@ -1172,9 +1223,11 @@ struct mlx5_flow_action {
.queue = 0,
.drop = 0,
.mark = 0,
+ .count = 0,
.mark_id = MLX5_FLOW_MARK_DEFAULT,
.queues_n = 0,
};
+ struct ibv_counter_set *counter = NULL;
int err;
err = priv_flow_validate(priv, attr, items, actions, error, &flow,
@@ -1205,12 +1258,23 @@ struct mlx5_flow_action {
mlx5_flow_create_flag_mark(&flow, action.mark_id);
flow.offset += sizeof(struct ibv_exp_flow_spec_action_tag);
}
+ if (action.count) {
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
+ counter = priv_flow_create_counter(priv,&flow,0);
+ if(!counter)
+ goto exit;
+ flow.offset += sizeof(struct ibv_exp_flow_spec_action_count);
+#else
+ goto exit;
+#endif
+ }
if (action.drop)
rte_flow =
priv_flow_create_action_queue_drop(priv, &flow, error);
else
rte_flow = priv_flow_create_action_queue(priv, &flow, &action,
error);
+ rte_flow->counter = counter;
if (!rte_flow)
goto exit;
return rte_flow;
@@ -1258,6 +1322,12 @@ struct rte_flow *
struct rte_flow *flow)
{
TAILQ_REMOVE(&priv->flows, flow, next);
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
+ if (flow->counter) {
+ claim_zero(ibv_destroy_counter_set(flow->counter));
+ flow->counter = NULL;
+ }
+#endif
if (flow->ibv_flow)
claim_zero(ibv_exp_destroy_flow(flow->ibv_flow));
if (flow->drop)
@@ -1602,3 +1672,94 @@ struct rte_flow *
priv_unlock(priv);
return 0;
}
+
+/**
+ * Internal function to read the counter
+ * value
+ *
+ * @param priv
+ * Pointer to private structure.
+ * @param counter_set
+ * Pointer to counter_set structure
+ * @param res
+ * Pointer to rte_flow_query_count struct
+ * used to return the values
+ * @param error
+ * returns the error.
+ *
+ * @return
+ * 0 success, -1 otherwise.
+ */
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_COUNT
+static int
+priv_flow_query_counter(struct priv *priv,
+ struct ibv_counter_set *counter_set,
+ struct rte_flow_query_count *res,
+ struct rte_flow_error *error)
+{
+ int res_value = 0;
+ uint64_t counters[2];
+ struct ibv_counter_set_description description;
+ struct ibv_query_counter_set_attr attr = { .counter_set = counter_set, };
+ res_value = ibv_query_counter_set(attr,counters);
+ if(res_value < 0) {
+ rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, "Error while querying counter set");
+ }
+ res->hits_set = 1;
+ res->hits = counters[0];
+ res->bytes_set = 1;
+ res->bytes = counters[1];
+ res_value = ibv_query_counter_set(attr,res->hits);
+ return res_value;
+}
+#endif
+
+/**
+ * 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;
+ }
+#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;
+#endif
+ break;
+ default:
+ rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL,
+ "Type not supported");
+ res_value = -1;
+ }
+ return res_value;
+}