Re: [dpdk-dev] [PATCH v3 6/6] net/mlx5: flow counters Verbs interface functions update

[Shahaf Shuler <shahafs@mellanox.com>]

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 <viacheslavo@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