[v3,6/6] net/mlx5: flow counters Verbs interface functions update
Checks
Commit Message
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(-)
Comments
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
@@ -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;
+#endif
uint64_t hits; /**< Number of packets matched by the rule. */
uint64_t bytes; /**< Number of bytes matched by the rule. */
};
@@ -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)
+/**
+ * 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.
+ *
+ * @return
+ * counter->cs contains a handle of created Verbs counter,
+ * NULL if error occurred and rte_errno is set.
+ */
+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)
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;