Friday, October 19, 2018 6:21 PM, Slava Ovsiienko:
> Subject: [PATCH v3 6/6] net/mlx5: flow counters Verbs interface functions
> update
How about: "net/mlx5: support new flow counter API"
>
> This part of patchset updates the functions performing the Verbs library calls
> in order to support different versions of library.
> The functions:
> - flow_verbs_counter_new()
> - flow_verbs_counter_release()
> - flow_verbs_counter_query()
> now have the several compilations branches, depending on the counter
> support found in the system at compile time.
>
> The flow_verbs_counter_create() function is introduced as helper for
> flow_verbs_counter_new(), actually this helper create the counters with
> Verbs.
>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_flow.h | 6 ++
> drivers/net/mlx5/mlx5_flow_verbs.c | 129
> ++++++++++++++++++++++++++++---------
> 2 files changed, 104 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 69f55cf..44c7515 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -224,7 +224,13 @@ struct mlx5_flow_counter {
> uint32_t shared:1; /**< Share counter ID with other flow rules. */
> uint32_t ref_cnt:31; /**< Reference counter. */
> uint32_t id; /**< Counter ID. */
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> struct ibv_counter_set *cs; /**< Holds the counters for the rule. */
> +#elif defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + struct ibv_counters *cs; /**< Holds the counters for the rule. */
> +#else
> + void *cs;
Why you need this else?
> +#endif
> uint64_t hits; /**< Number of packets matched by the rule. */
> uint64_t bytes; /**< Number of bytes matched by the rule. */ }; diff
> --git a/drivers/net/mlx5/mlx5_flow_verbs.c
> b/drivers/net/mlx5/mlx5_flow_verbs.c
> index f720c35..b657933 100644
> --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> @@ -33,6 +33,59 @@
> #include "mlx5_glue.h"
> #include "mlx5_flow.h"
>
> +
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \
> + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
As I already commented on the previous version. Ifdef are always inside the
function body, and not before the function declaration.
If no support for counters the function should return errno.
> +/**
> + * Create Verbs flow counter with Verbs library.
> + *
> + * @param[in] dev
> + * Pointer to the Ethernet device structure.
> + * @param[in, out] counter
> + * PMD flow counter object, contains the counter id,
> + * handle of created Verbs flow counter is returned in cs field.
The struct maybe will change in the future, but the doc isn't. better to say
"mlx5 flow counter object"
> + *
> + * @return
> + * counter->cs contains a handle of created Verbs counter,
> + * NULL if error occurred and rte_errno is set.
How can you return NULL if the function returns void?
It seems this function needs to return an integer.
> + */
> +static void
> +flow_verbs_counter_create(struct rte_eth_dev *dev,
> + struct mlx5_flow_counter *counter) { #if
> +defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> + struct priv *priv = dev->data->dev_private;
> + struct ibv_counter_set_init_attr init = {
> + .counter_set_id = counter->id};
> +
> + counter->cs = mlx5_glue->create_counter_set(priv->ctx, &init); #elif
> +defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + struct priv *priv = dev->data->dev_private;
> + struct ibv_counters_init_attr init = {0};
> + struct ibv_counter_attach_attr attach = {0};
> + int ret;
> +
> + counter->cs = mlx5_glue->create_counters(priv->ctx, &init);
> + if (!counter->cs)
> + return;
> + attach.counter_desc = IBV_COUNTER_PACKETS;
> + attach.index = 0;
> + ret = mlx5_glue->attach_counters(counter->cs, &attach, NULL);
> + if (!ret) {
> + attach.counter_desc = IBV_COUNTER_BYTES;
> + attach.index = 1;
> + ret = mlx5_glue->attach_counters
> + (counter->cs, &attach, NULL);
> + }
> + if (ret) {
> + claim_zero(mlx5_glue->destroy_counters(counter->cs));
> + counter->cs = NULL;
> + rte_errno = ret;
> + }
> +#endif
> +}
> +#endif
> +
> /**
> * Get a flow counter.
> *
> @@ -49,6 +102,8 @@
> static struct mlx5_flow_counter *
> flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared,
> uint32_t id) {
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \
> + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
No explicit need for this ifdef, the flow_verbs_counter_create will return
error if the counters are not supported. This is a duplicate check.
> struct priv *priv = dev->data->dev_private;
> struct mlx5_flow_counter *cnt;
>
> @@ -60,37 +115,32 @@
> cnt->ref_cnt++;
> return cnt;
> }
> -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42
> -
> - struct mlx5_flow_counter tmpl = {
> - .shared = shared,
> - .id = id,
> - .cs = mlx5_glue->create_counter_set
> - (priv->ctx,
> - &(struct ibv_counter_set_init_attr){
> - .counter_set_id = id,
> - }),
> - .hits = 0,
> - .bytes = 0,
> - .ref_cnt = 1,
> - };
> -
> - if (!tmpl.cs) {
> - rte_errno = errno;
> - return NULL;
> - }
> cnt = rte_calloc(__func__, 1, sizeof(*cnt), 0);
> if (!cnt) {
> - claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
> rte_errno = ENOMEM;
> return NULL;
> }
> - *cnt = tmpl;
> - LIST_INSERT_HEAD(&priv->flow_counters, cnt, next);
> - return cnt;
> -#endif
> + cnt->id = id;
> + cnt->shared = shared;
> + cnt->ref_cnt = 1;
> + cnt->hits = 0;
> + cnt->bytes = 0;
> + /* Create counter with Verbs. */
> + flow_verbs_counter_create(dev, cnt);
> + if (cnt->cs) {
> + LIST_INSERT_HEAD(&priv->flow_counters, cnt, next);
> + return cnt;
> + }
> + /* Some error occurred in Verbs library, rte_errno is set. */
> + rte_free(cnt);
> + return NULL;
> +#else
> + (void)dev;
> + (void)shared;
> + (void)id;
> rte_errno = ENOTSUP;
> return NULL;
> +#endif
> }
>
> /**
> @@ -103,7 +153,11 @@
> flow_verbs_counter_release(struct mlx5_flow_counter *counter) {
> if (--counter->ref_cnt == 0) {
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> claim_zero(mlx5_glue->destroy_counter_set(counter->cs));
> +#elif defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + claim_zero(mlx5_glue->destroy_counters(counter->cs));
> +#endif
> LIST_REMOVE(counter, next);
> rte_free(counter);
> }
> @@ -117,14 +171,15 @@
> */
> static int
> flow_verbs_counter_query(struct rte_eth_dev *dev __rte_unused,
> - struct rte_flow *flow __rte_unused,
> - void *data __rte_unused,
> + struct rte_flow *flow, void *data,
> struct rte_flow_error *error)
> {
> -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \
> + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> if (flow->actions & MLX5_FLOW_ACTION_COUNT) {
> struct rte_flow_query_count *qc = data;
> uint64_t counters[2] = {0, 0};
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> struct ibv_query_counter_set_attr query_cs_attr = {
> .cs = flow->counter->cs,
> .query_flags = IBV_COUNTER_SET_FORCE_UPDATE,
> @@ -135,7 +190,12 @@
> };
> int err = mlx5_glue->query_counter_set(&query_cs_attr,
> &query_out);
> -
> +#elif defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + int err = mlx5_glue->query_counters(
> + flow->counter->cs, counters,
> + RTE_DIM(counters),
> +
> IBV_READ_COUNTERS_ATTR_PREFER_CACHED);
> +#endif
> if (err)
> return rte_flow_error_set
> (error, err,
> @@ -157,6 +217,8 @@
> NULL,
> "flow does not have counter");
> #else
> + (void)flow;
> + (void)data;
> return rte_flow_error_set(error, ENOTSUP,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> NULL,
> @@ -993,7 +1055,8 @@
> {
> const struct rte_flow_action_count *count = action->conf;
> struct rte_flow *flow = dev_flow->flow; -#ifdef
> HAVE_IBV_DEVICE_COUNTERS_SET_V42
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \
> + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> unsigned int size = sizeof(struct ibv_flow_spec_counter_action);
> struct ibv_flow_spec_counter_action counter = {
> .type = IBV_FLOW_SPEC_ACTION_COUNT,
> @@ -1012,9 +1075,12 @@
> " context.");
> }
> *action_flags |= MLX5_FLOW_ACTION_COUNT; -#ifdef
> HAVE_IBV_DEVICE_COUNTERS_SET_V42
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> counter.counter_set_handle = flow->counter->cs->handle;
> flow_verbs_spec_add(dev_flow, &counter, size);
> +#elif defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + counter.counters = flow->counter->cs;
> + flow_verbs_spec_add(dev_flow, &counter, size);
> #endif
> return 0;
> }
> @@ -1277,7 +1343,8 @@
> detected_actions |= MLX5_FLOW_ACTION_RSS;
> break;
> case RTE_FLOW_ACTION_TYPE_COUNT:
> -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \
> + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> size += sizeof(struct ibv_flow_spec_counter_action);
> #endif
> detected_actions |= MLX5_FLOW_ACTION_COUNT;
> --
> 1.8.3.1
