net/mlx5: flow counters support on the Linux-rdma v19 base
Checks
Commit Message
Mellanox mlx5 PMD supports Flow Counters via Verbs library.
The current implementation is based on the Mellanox proprietary
Verbs library included in MLNX OFED packages. The Flow Counter
support is recently added into linux-rdma release (v19),
so the mlx5 PMD update is needed to provide Counter feature
on the base of linux-rdma.
mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+
and provide flow counters for both.
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
drivers/net/mlx5/Makefile | 10 ++++++++
drivers/net/mlx5/mlx5.c | 6 +++++
drivers/net/mlx5/mlx5_flow.c | 9 ++++++-
drivers/net/mlx5/mlx5_flow.h | 4 +++
drivers/net/mlx5/mlx5_flow_verbs.c | 52 ++++++++++++++++++++++++++++++--------
drivers/net/mlx5/mlx5_glue.c | 41 ++++++++++++++++++++++++++++++
drivers/net/mlx5/mlx5_glue.h | 16 ++++++++++++
7 files changed, 127 insertions(+), 11 deletions(-)
Comments
On Wed, Oct 03, 2018 at 03:29:12PM +0000, Slava Ovsiienko wrote:
> Mellanox mlx5 PMD supports Flow Counters via Verbs library.
> The current implementation is based on the Mellanox proprietary
> Verbs library included in MLNX OFED packages. The Flow Counter
> support is recently added into linux-rdma release (v19),
> so the mlx5 PMD update is needed to provide Counter feature
> on the base of linux-rdma.
>
> mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+
> and provide flow counters for both.
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
> drivers/net/mlx5/Makefile | 10 ++++++++
> drivers/net/mlx5/mlx5.c | 6 +++++
> drivers/net/mlx5/mlx5_flow.c | 9 ++++++-
> drivers/net/mlx5/mlx5_flow.h | 4 +++
> drivers/net/mlx5/mlx5_flow_verbs.c | 52 ++++++++++++++++++++++++++++++--------
> drivers/net/mlx5/mlx5_glue.c | 41 ++++++++++++++++++++++++++++++
> drivers/net/mlx5/mlx5_glue.h | 16 ++++++++++++
> 7 files changed, 127 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> index ca1de9f..e3d2156 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -162,6 +162,16 @@ mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
> type 'struct ibv_counter_set_init_attr' \
> $(AUTOCONF_OUTPUT)
> $Q sh -- '$<' '$@' \
> + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> + infiniband/verbs.h \
> + type 'struct ibv_counters_init_attr' \
> + $(AUTOCONF_OUTPUT)
> + $Q sh -- '$<' '$@' \
> + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
> + infiniband/verbs.h \
> + type 'struct ibv_counters_init_attr' \
> + $(AUTOCONF_OUTPUT)
> + $Q sh -- '$<' '$@' \
I still don't understand what is different between the two. These are exactly
same checking, then why do you need to have two different macros? From this
script, HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is same as
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, isn't it?
And if you make some changes in Makefile, you should also make corresponding
changes for the meson build.
> HAVE_RDMA_NL_NLDEV \
> rdma/rdma_netlink.h \
> enum RDMA_NL_NLDEV \
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 4be6a1c..81f6ba1 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -739,8 +739,10 @@
> unsigned int mprq_min_stride_num_n = 0;
> unsigned int mprq_max_stride_num_n = 0;
> #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> struct ibv_counter_set_description cs_desc = { .counter_type = 0 };
> #endif
> +#endif
> struct ether_addr mac;
> char name[RTE_ETH_NAME_MAX_LEN];
> int own_domain_id = 0;
> @@ -1009,11 +1011,15 @@
> DRV_LOG(DEBUG, "checksum offloading is %ssupported",
> (config.hw_csum ? "" : "not "));
> #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> config.flow_counter_en = !!attr.max_counter_sets;
> mlx5_glue->describe_counter_set(ctx, 0, &cs_desc);
> DRV_LOG(DEBUG, "counter type = %d, num of cs = %ld, attributes = %d",
> cs_desc.counter_type, cs_desc.num_of_cs,
> cs_desc.attributes);
> +#else
> + config.flow_counter_en = 1;
> +#endif
> #endif
> config.ind_table_max_size =
> attr.rss_caps.max_rwq_indirection_table_size;
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 8007bf1..652580c 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2306,6 +2306,13 @@ struct rte_flow *
> if (flow->actions & MLX5_ACTION_COUNT) {
> struct rte_flow_query_count *qc = data;
> uint64_t counters[2] = {0, 0};
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + int err = mlx5_glue->query_counter_set(
> + flow->counter->cs,
> + counters,
> + RTE_DIM(counters),
> + IBV_READ_COUNTERS_ATTR_PREFER_CACHED);
> +#else
> struct ibv_query_counter_set_attr query_cs_attr = {
> .cs = flow->counter->cs,
> .query_flags = IBV_COUNTER_SET_FORCE_UPDATE,
> @@ -2316,7 +2323,7 @@ struct rte_flow *
> };
> int err = mlx5_glue->query_counter_set(&query_cs_attr,
> &query_out);
> -
> +#endif
> if (err)
> return rte_flow_error_set
> (error, err,
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 10d700a..a3b82dd 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -222,7 +222,11 @@ 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. */
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + struct ibv_counters *cs; /**< Holds the counters for the rule. */
> +#else
> struct ibv_counter_set *cs; /**< Holds the counters for the rule. */
> +#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 05ab5fd..1c8bdba 100644
> --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> @@ -48,27 +48,32 @@
> static struct mlx5_flow_counter *
> flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id)
> {
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> struct priv *priv = dev->data->dev_private;
> struct mlx5_flow_counter *cnt;
>
> - LIST_FOREACH(cnt, &priv->flow_counters, next) {
> - if (!cnt->shared || cnt->shared != shared)
> - continue;
> - if (cnt->id != id)
> - continue;
> - cnt->ref_cnt++;
> - return cnt;
> + if (shared) {
> + LIST_FOREACH(cnt, &priv->flow_counters, next)
> + if (cnt->shared && cnt->id == id) {
> + cnt->ref_cnt++;
> + return cnt;
> + }
> }
> -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
>
> struct mlx5_flow_counter tmpl = {
> .shared = shared,
> .id = id,
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + .cs = mlx5_glue->create_counter_set
> + (priv->ctx,
> + &(struct ibv_counters_init_attr){0}),
> +#else
> .cs = mlx5_glue->create_counter_set
> (priv->ctx,
> &(struct ibv_counter_set_init_attr){
> .counter_set_id = id,
> }),
> +#endif
> .hits = 0,
> .bytes = 0,
> };
> @@ -77,17 +82,40 @@
> rte_errno = errno;
> return NULL;
> }
> +
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + struct ibv_counter_attach_attr attach_attr = {0};
> + int ret;
> +
> + attach_attr.counter_desc = IBV_COUNTER_PACKETS;
> + attach_attr.index = 0;
> + ret = ibv_attach_counters_point_flow(tmpl.cs, &attach_attr, NULL);
> + if (!ret) {
> + attach_attr.counter_desc = IBV_COUNTER_BYTES;
> + attach_attr.index = 1;
> + ret = ibv_attach_counters_point_flow(tmpl.cs,
> + &attach_attr,
> + NULL);
> + }
> + if (ret) {
> + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
> + rte_errno = ret;
> + return NULL;
> + }
> +#endif
> 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
> +#else
> rte_errno = ENOTSUP;
> return NULL;
> +#endif
> }
>
> /**
> @@ -947,7 +975,7 @@
> flow->counter = flow_verbs_counter_new(dev, count->shared,
> count->id);
> if (!flow->counter)
> - return rte_flow_error_set(error, ENOTSUP,
> + return rte_flow_error_set(error, rte_errno,
> RTE_FLOW_ERROR_TYPE_ACTION,
> action,
> "cannot get counter"
> @@ -955,7 +983,11 @@
> }
> *action_flags |= MLX5_ACTION_COUNT;
> #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + counter.counters = flow->counter->cs;
> +#else
> counter.counter_set_handle = flow->counter->cs->handle;
> +#endif
> flow_verbs_spec_add(dev_flow, &counter, size);
> #endif
> return 0;
> diff --git a/drivers/net/mlx5/mlx5_glue.c b/drivers/net/mlx5/mlx5_glue.c
> index 48590df..785234c 100644
> --- a/drivers/net/mlx5/mlx5_glue.c
> +++ b/drivers/net/mlx5/mlx5_glue.c
> @@ -211,6 +211,39 @@
> return ibv_dereg_mr(mr);
> }
>
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> +static struct ibv_counters *
> +mlx5_glue_create_counters(struct ibv_context *context,
> + struct ibv_counters_init_attr *init_attr)
> +{
> + return ibv_create_counters(context, init_attr);
> +}
> +
> +static int
> +mlx5_glue_destroy_counters(struct ibv_counters *counters)
> +{
> + return ibv_destroy_counters(counters);
> +}
> +
> +static int
> +mlx5_glue_attach_counters(struct ibv_counters *counters,
> + struct ibv_counter_attach_attr *attr,
> + struct ibv_flow *flow)
> +{
> + return ibv_attach_counters_point_flow(counters, attr, flow);
> +}
> +
> +static int
> +mlx5_glue_query_counters(struct ibv_counters *counters,
> + uint64_t *counters_value,
> + uint32_t ncounters,
> + uint32_t flags)
> +{
> + return ibv_read_counters(counters, counters_value, ncounters, flags);
> +}
> +#endif
> +
> +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> static struct ibv_counter_set *
> mlx5_glue_create_counter_set(struct ibv_context *context,
> struct ibv_counter_set_init_attr *init_attr)
> @@ -262,6 +295,7 @@
> return ibv_query_counter_set(query_attr, cs_data);
> #endif
> }
> +#endif
>
> static void
> mlx5_glue_ack_async_event(struct ibv_async_event *event)
> @@ -420,10 +454,17 @@
> .modify_qp = mlx5_glue_modify_qp,
> .reg_mr = mlx5_glue_reg_mr,
> .dereg_mr = mlx5_glue_dereg_mr,
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + .create_counter_set = mlx5_glue_create_counters,
> + .destroy_counter_set = mlx5_glue_destroy_counters,
> + .attach_counter_set = mlx5_glue_attach_counters,
> + .query_counter_set = mlx5_glue_query_counters,
> +#else
> .create_counter_set = mlx5_glue_create_counter_set,
> .destroy_counter_set = mlx5_glue_destroy_counter_set,
> .describe_counter_set = mlx5_glue_describe_counter_set,
> .query_counter_set = mlx5_glue_query_counter_set,
> +#endif
> .ack_async_event = mlx5_glue_ack_async_event,
> .get_async_event = mlx5_glue_get_async_event,
> .port_state_str = mlx5_glue_port_state_str,
> diff --git a/drivers/net/mlx5/mlx5_glue.h b/drivers/net/mlx5/mlx5_glue.h
> index f6e4e38..504d487 100644
> --- a/drivers/net/mlx5/mlx5_glue.h
> +++ b/drivers/net/mlx5/mlx5_glue.h
> @@ -96,6 +96,21 @@ struct mlx5_glue {
> struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr,
> size_t length, int access);
> int (*dereg_mr)(struct ibv_mr *mr);
> +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> + struct ibv_counters *(*create_counter_set)
> + (struct ibv_context *context,
> + struct ibv_counters_init_attr *init_attr);
> + int (*destroy_counter_set)(struct ibv_counters *cs);
> + int (*attach_counter_set)
> + (struct ibv_counters *cs,
> + struct ibv_counter_attach_attr *attr,
> + struct ibv_flow *flow);
> + int (*query_counter_set)
> + (struct ibv_counters *cs,
> + uint64_t *counters_value,
> + uint32_t ncounters,
> + uint32_t flags);
> +#else
> struct ibv_counter_set *(*create_counter_set)
> (struct ibv_context *context,
> struct ibv_counter_set_init_attr *init_attr);
> @@ -106,6 +121,7 @@ struct mlx5_glue {
> struct ibv_counter_set_description *cs_desc);
> int (*query_counter_set)(struct ibv_query_counter_set_attr *query_attr,
> struct ibv_counter_set_data *cs_data);
> +#endif
> void (*ack_async_event)(struct ibv_async_event *event);
> int (*get_async_event)(struct ibv_context *context,
> struct ibv_async_event *event);
> --
> 1.8.3.1
>
Hi Slava,
Adding some more comments,
Thursday, October 4, 2018 2:48 AM, Yongseok Koh:
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> Linux-rdma v19 base
>
> On Wed, Oct 03, 2018 at 03:29:12PM +0000, Slava Ovsiienko wrote:
> > Mellanox mlx5 PMD supports Flow Counters via Verbs library.
> > The current implementation is based on the Mellanox proprietary Verbs
> > library included in MLNX OFED packages. The Flow Counter support is
> > recently added into linux-rdma release (v19), so the mlx5 PMD update
> > is needed to provide Counter feature on the base of linux-rdma.
> >
> > mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+ and
> provide
> > flow counters for both.
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> > drivers/net/mlx5/Makefile | 10 ++++++++
> > drivers/net/mlx5/mlx5.c | 6 +++++
> > drivers/net/mlx5/mlx5_flow.c | 9 ++++++-
> > drivers/net/mlx5/mlx5_flow.h | 4 +++
> > drivers/net/mlx5/mlx5_flow_verbs.c | 52
> ++++++++++++++++++++++++++++++--------
> > drivers/net/mlx5/mlx5_glue.c | 41
> ++++++++++++++++++++++++++++++
> > drivers/net/mlx5/mlx5_glue.h | 16 ++++++++++++
> > 7 files changed, 127 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> > index ca1de9f..e3d2156 100644
> > --- a/drivers/net/mlx5/Makefile
> > +++ b/drivers/net/mlx5/Makefile
> > @@ -162,6 +162,16 @@ mlx5_autoconf.h.new:
> $(RTE_SDK)/buildtools/auto-config-h.sh
> > type 'struct ibv_counter_set_init_attr' \
> > $(AUTOCONF_OUTPUT)
> > $Q sh -- '$<' '$@' \
> > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> > + infiniband/verbs.h \
> > + type 'struct ibv_counters_init_attr' \
> > + $(AUTOCONF_OUTPUT)
> > + $Q sh -- '$<' '$@' \
> > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
> > + infiniband/verbs.h \
> > + type 'struct ibv_counters_init_attr' \
> > + $(AUTOCONF_OUTPUT)
> > + $Q sh -- '$<' '$@' \
>
> I still don't understand what is different between the two. These are exactly
> same checking, then why do you need to have two different macros? From
> this script, HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is same as
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, isn't it?
>
> And if you make some changes in Makefile, you should also make
> corresponding changes for the meson build.
>
> > HAVE_RDMA_NL_NLDEV \
> > rdma/rdma_netlink.h \
> > enum RDMA_NL_NLDEV \
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 4be6a1c..81f6ba1 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -739,8 +739,10 @@
> > unsigned int mprq_min_stride_num_n = 0;
> > unsigned int mprq_max_stride_num_n = 0; #ifdef
> > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
Is there really a rdma-core version on which both types of counters exists together?
> > struct ibv_counter_set_description cs_desc = { .counter_type = 0 };
> > #endif
> > +#endif
> > struct ether_addr mac;
> > char name[RTE_ETH_NAME_MAX_LEN];
> > int own_domain_id = 0;
> > @@ -1009,11 +1011,15 @@
> > DRV_LOG(DEBUG, "checksum offloading is %ssupported",
> > (config.hw_csum ? "" : "not "));
> > #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > config.flow_counter_en = !!attr.max_counter_sets;
> > mlx5_glue->describe_counter_set(ctx, 0, &cs_desc);
> > DRV_LOG(DEBUG, "counter type = %d, num of cs = %ld, attributes =
> %d",
> > cs_desc.counter_type, cs_desc.num_of_cs,
> > cs_desc.attributes);
> > +#else
> > + config.flow_counter_en = 1;
> > +#endif
> > #endif
> > config.ind_table_max_size =
> > attr.rss_caps.max_rwq_indirection_table_size;
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 8007bf1..652580c 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2306,6 +2306,13 @@ struct rte_flow *
> > if (flow->actions & MLX5_ACTION_COUNT) {
> > struct rte_flow_query_count *qc = data;
> > uint64_t counters[2] = {0, 0};
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + int err = mlx5_glue->query_counter_set(
> > + flow->counter->cs,
> > + counters,
> > + RTE_DIM(counters),
> > +
> IBV_READ_COUNTERS_ATTR_PREFER_CACHED);
> > +#else
This part is under #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT. So on rdma-core versions were the "old" counters are not supported anymore this code wil never be reached.
> > struct ibv_query_counter_set_attr query_cs_attr = {
> > .cs = flow->counter->cs,
> > .query_flags = IBV_COUNTER_SET_FORCE_UPDATE,
> @@ -2316,7 +2323,7 @@
> > struct rte_flow *
> > };
> > int err = mlx5_glue->query_counter_set(&query_cs_attr,
> > &query_out);
> > -
> > +#endif
> > if (err)
> > return rte_flow_error_set
> > (error, err,
> > diff --git a/drivers/net/mlx5/mlx5_flow.h
> > b/drivers/net/mlx5/mlx5_flow.h index 10d700a..a3b82dd 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -222,7 +222,11 @@ 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. */
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counters *cs; /**< Holds the counters for the rule. */
> > +#else
> > struct ibv_counter_set *cs; /**< Holds the counters for the rule. */
> > +#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 05ab5fd..1c8bdba 100644
> > --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> > @@ -48,27 +48,32 @@
> > static struct mlx5_flow_counter *
> > flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared,
> > uint32_t id) {
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
I don't understand why you choose to move the ifdef.
> > struct priv *priv = dev->data->dev_private;
> > struct mlx5_flow_counter *cnt;
> >
> > - LIST_FOREACH(cnt, &priv->flow_counters, next) {
> > - if (!cnt->shared || cnt->shared != shared)
> > - continue;
> > - if (cnt->id != id)
> > - continue;
> > - cnt->ref_cnt++;
> > - return cnt;
> > + if (shared) {
> > + LIST_FOREACH(cnt, &priv->flow_counters, next)
> > + if (cnt->shared && cnt->id == id) {
> > + cnt->ref_cnt++;
> > + return cnt;
> > + }
Nor this code. Is it related to the support patch or you are fixing bug on the way. If bug fix it deserve a separate commit.
> > }
> > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> >
> > struct mlx5_flow_counter tmpl = {
> > .shared = shared,
> > .id = id,
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
Same - you will never reach this code if the old flow count is not supported on the underlying rdma-core version.
> > + .cs = mlx5_glue->create_counter_set
> > + (priv->ctx,
> > + &(struct ibv_counters_init_attr){0}), #else
> > .cs = mlx5_glue->create_counter_set
> > (priv->ctx,
> > &(struct ibv_counter_set_init_attr){
> > .counter_set_id = id,
> > }),
Here from the old counters perspective the counter is created...
> > +#endif
> > .hits = 0,
> > .bytes = 0,
> > };
> > @@ -77,17 +82,40 @@
> > rte_errno = errno;
> > return NULL;
> > }
> > +
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counter_attach_attr attach_attr = {0};
> > + int ret;
> > +
> > + attach_attr.counter_desc = IBV_COUNTER_PACKETS;
> > + attach_attr.index = 0;
> > + ret = ibv_attach_counters_point_flow(tmpl.cs, &attach_attr, NULL);
> > + if (!ret) {
> > + attach_attr.counter_desc = IBV_COUNTER_BYTES;
> > + attach_attr.index = 1;
> > + ret = ibv_attach_counters_point_flow(tmpl.cs,
> > + &attach_attr,
> > + NULL);
Here it is the end of the counter creation on the new API. My suggestion is to have a single ifdef which create the new/old counter (and not two).
> > + }
> > + if (ret) {
> > + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
> > + rte_errno = ret;
> > + return NULL;
> > + }
> > +#endif
> > cnt = rte_calloc(__func__, 1, sizeof(*cnt), 0);
> > if (!cnt) {
> > + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
Another bug fix?
> > rte_errno = ENOMEM;
> > return NULL;
> > }
> > *cnt = tmpl;
> > LIST_INSERT_HEAD(&priv->flow_counters, cnt, next);
> > return cnt;
> > -#endif
> > +#else
> > rte_errno = ENOTSUP;
> > return NULL;
> > +#endif
> > }
> >
> > /**
> > @@ -947,7 +975,7 @@
> > flow->counter = flow_verbs_counter_new(dev, count-
> >shared,
> > count->id);
> > if (!flow->counter)
> > - return rte_flow_error_set(error, ENOTSUP,
> > + return rte_flow_error_set(error, rte_errno,
> >
> RTE_FLOW_ERROR_TYPE_ACTION,
> > action,
> > "cannot get counter"
> > @@ -955,7 +983,11 @@
> > }
> > *action_flags |= MLX5_ACTION_COUNT;
> > #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
Same comment about the code never reached.
> > + counter.counters = flow->counter->cs; #else
> > counter.counter_set_handle = flow->counter->cs->handle;
> > +#endif
> > flow_verbs_spec_add(dev_flow, &counter, size); #endif
> > return 0;
> > diff --git a/drivers/net/mlx5/mlx5_glue.c
> > b/drivers/net/mlx5/mlx5_glue.c index 48590df..785234c 100644
> > --- a/drivers/net/mlx5/mlx5_glue.c
> > +++ b/drivers/net/mlx5/mlx5_glue.c
> > @@ -211,6 +211,39 @@
> > return ibv_dereg_mr(mr);
> > }
> >
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > +static struct ibv_counters *
> > +mlx5_glue_create_counters(struct ibv_context *context,
> > + struct ibv_counters_init_attr *init_attr) {
> > + return ibv_create_counters(context, init_attr); }
> > +
> > +static int
> > +mlx5_glue_destroy_counters(struct ibv_counters *counters) {
> > + return ibv_destroy_counters(counters); }
> > +
> > +static int
> > +mlx5_glue_attach_counters(struct ibv_counters *counters,
> > + struct ibv_counter_attach_attr *attr,
> > + struct ibv_flow *flow)
> > +{
> > + return ibv_attach_counters_point_flow(counters, attr, flow); }
> > +
> > +static int
> > +mlx5_glue_query_counters(struct ibv_counters *counters,
> > + uint64_t *counters_value,
> > + uint32_t ncounters,
> > + uint32_t flags)
> > +{
> > + return ibv_read_counters(counters, counters_value, ncounters,
> > +flags); } #endif
> > +
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > static struct ibv_counter_set *
> > mlx5_glue_create_counter_set(struct ibv_context *context,
> > struct ibv_counter_set_init_attr *init_attr) @@ -
> 262,6 +295,7
> > @@
> > return ibv_query_counter_set(query_attr, cs_data); #endif }
> > +#endif
> >
> > static void
> > mlx5_glue_ack_async_event(struct ibv_async_event *event) @@ -420,10
> > +454,17 @@
> > .modify_qp = mlx5_glue_modify_qp,
> > .reg_mr = mlx5_glue_reg_mr,
> > .dereg_mr = mlx5_glue_dereg_mr,
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + .create_counter_set = mlx5_glue_create_counters,
> > + .destroy_counter_set = mlx5_glue_destroy_counters,
> > + .attach_counter_set = mlx5_glue_attach_counters,
> > + .query_counter_set = mlx5_glue_query_counters, #else
> > .create_counter_set = mlx5_glue_create_counter_set,
> > .destroy_counter_set = mlx5_glue_destroy_counter_set,
> > .describe_counter_set = mlx5_glue_describe_counter_set,
> > .query_counter_set = mlx5_glue_query_counter_set,
> > +#endif
> > .ack_async_event = mlx5_glue_ack_async_event,
> > .get_async_event = mlx5_glue_get_async_event,
> > .port_state_str = mlx5_glue_port_state_str, diff --git
> > a/drivers/net/mlx5/mlx5_glue.h b/drivers/net/mlx5/mlx5_glue.h index
> > f6e4e38..504d487 100644
> > --- a/drivers/net/mlx5/mlx5_glue.h
> > +++ b/drivers/net/mlx5/mlx5_glue.h
> > @@ -96,6 +96,21 @@ struct mlx5_glue {
> > struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr,
> > size_t length, int access);
> > int (*dereg_mr)(struct ibv_mr *mr);
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counters *(*create_counter_set)
> > + (struct ibv_context *context,
> > + struct ibv_counters_init_attr *init_attr);
> > + int (*destroy_counter_set)(struct ibv_counters *cs);
> > + int (*attach_counter_set)
> > + (struct ibv_counters *cs,
> > + struct ibv_counter_attach_attr *attr,
> > + struct ibv_flow *flow);
> > + int (*query_counter_set)
> > + (struct ibv_counters *cs,
> > + uint64_t *counters_value,
> > + uint32_t ncounters,
> > + uint32_t flags);
> > +#else
> > struct ibv_counter_set *(*create_counter_set)
> > (struct ibv_context *context,
> > struct ibv_counter_set_init_attr *init_attr); @@ -106,6
> +121,7 @@
> > struct mlx5_glue {
> > struct ibv_counter_set_description *cs_desc);
> > int (*query_counter_set)(struct ibv_query_counter_set_attr
> *query_attr,
> > struct ibv_counter_set_data *cs_data);
> > +#endif
Pay attention to how the old counter supported was added to the mlx5_glue lib.
There are no ifdefs of the function pointer declaration nor on the function declaration. This was because it was causing link issues between this lib to verbs.
Same should be for the new counters. Need to declare all needed structs if not declared on the mlx5_glue.h file, and always the function pointer and the declaration.
The ifdef is only inside the function implementation.
> > void (*ack_async_event)(struct ibv_async_event *event);
> > int (*get_async_event)(struct ibv_context *context,
> > struct ibv_async_event *event);
> > --
> > 1.8.3.1
> >
> -----Original Message-----
> From: Yongseok Koh
> Sent: Thursday, October 4, 2018 2:48
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> Linux-rdma v19 base
>
> On Wed, Oct 03, 2018 at 03:29:12PM +0000, Slava Ovsiienko wrote:
> > Mellanox mlx5 PMD supports Flow Counters via Verbs library.
> > The current implementation is based on the Mellanox proprietary Verbs
> > library included in MLNX OFED packages. The Flow Counter support is
> > recently added into linux-rdma release (v19), so the mlx5 PMD update
> > is needed to provide Counter feature on the base of linux-rdma.
> >
> > mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+ and
> provide
> > flow counters for both.
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> > drivers/net/mlx5/Makefile | 10 ++++++++
> > drivers/net/mlx5/mlx5.c | 6 +++++
> > drivers/net/mlx5/mlx5_flow.c | 9 ++++++-
> > drivers/net/mlx5/mlx5_flow.h | 4 +++
> > drivers/net/mlx5/mlx5_flow_verbs.c | 52
> ++++++++++++++++++++++++++++++--------
> > drivers/net/mlx5/mlx5_glue.c | 41
> ++++++++++++++++++++++++++++++
> > drivers/net/mlx5/mlx5_glue.h | 16 ++++++++++++
> > 7 files changed, 127 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> > index ca1de9f..e3d2156 100644
> > --- a/drivers/net/mlx5/Makefile
> > +++ b/drivers/net/mlx5/Makefile
> > @@ -162,6 +162,16 @@ mlx5_autoconf.h.new:
> $(RTE_SDK)/buildtools/auto-config-h.sh
> > type 'struct ibv_counter_set_init_attr' \
> > $(AUTOCONF_OUTPUT)
> > $Q sh -- '$<' '$@' \
> > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> > + infiniband/verbs.h \
> > + type 'struct ibv_counters_init_attr' \
> > + $(AUTOCONF_OUTPUT)
> > + $Q sh -- '$<' '$@' \
> > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
> > + infiniband/verbs.h \
> > + type 'struct ibv_counters_init_attr' \
> > + $(AUTOCONF_OUTPUT)
> > + $Q sh -- '$<' '$@' \
>
> I still don't understand what is different between the two. These are exactly
> same checking, then why do you need to have two different macros? From
> this script, HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is same as
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, isn't it?
We have three options:
- no counter support in kernel at all
- "old" counter support (ibv_counter_set_init_attr is defined in verbs.h)
- "new" counter support (ibv_counters_init_attr is defined in verbs.h)
Three options require at least two compilations flags. The meanings are chosen:
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT means there is counter support (of any type)
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 differentiates the support type
This approach allows to avoid clumsy constructions in code like this:
#if __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
|| __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45)
If there is no counter support in kernel at all
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is NOT defined
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 is NOT defined
if kernel provides "old counters"
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is defined
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 is NOT defined
if kernel provides "new counters"
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is defined
HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 is defined
>
> And if you make some changes in Makefile, you should also make
> corresponding changes for the meson build.
>
> > HAVE_RDMA_NL_NLDEV \
> > rdma/rdma_netlink.h \
> > enum RDMA_NL_NLDEV \
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 4be6a1c..81f6ba1 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -739,8 +739,10 @@
> > unsigned int mprq_min_stride_num_n = 0;
> > unsigned int mprq_max_stride_num_n = 0; #ifdef
> > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > struct ibv_counter_set_description cs_desc = { .counter_type = 0 };
> > #endif
> > +#endif
> > struct ether_addr mac;
> > char name[RTE_ETH_NAME_MAX_LEN];
> > int own_domain_id = 0;
> > @@ -1009,11 +1011,15 @@
> > DRV_LOG(DEBUG, "checksum offloading is %ssupported",
> > (config.hw_csum ? "" : "not "));
> > #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > config.flow_counter_en = !!attr.max_counter_sets;
> > mlx5_glue->describe_counter_set(ctx, 0, &cs_desc);
> > DRV_LOG(DEBUG, "counter type = %d, num of cs = %ld, attributes =
> %d",
> > cs_desc.counter_type, cs_desc.num_of_cs,
> > cs_desc.attributes);
> > +#else
> > + config.flow_counter_en = 1;
> > +#endif
> > #endif
> > config.ind_table_max_size =
> > attr.rss_caps.max_rwq_indirection_table_size;
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 8007bf1..652580c 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2306,6 +2306,13 @@ struct rte_flow *
> > if (flow->actions & MLX5_ACTION_COUNT) {
> > struct rte_flow_query_count *qc = data;
> > uint64_t counters[2] = {0, 0};
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + int err = mlx5_glue->query_counter_set(
> > + flow->counter->cs,
> > + counters,
> > + RTE_DIM(counters),
> > +
> IBV_READ_COUNTERS_ATTR_PREFER_CACHED);
> > +#else
> > struct ibv_query_counter_set_attr query_cs_attr = {
> > .cs = flow->counter->cs,
> > .query_flags = IBV_COUNTER_SET_FORCE_UPDATE,
> @@ -2316,7 +2323,7 @@
> > struct rte_flow *
> > };
> > int err = mlx5_glue->query_counter_set(&query_cs_attr,
> > &query_out);
> > -
> > +#endif
> > if (err)
> > return rte_flow_error_set
> > (error, err,
> > diff --git a/drivers/net/mlx5/mlx5_flow.h
> > b/drivers/net/mlx5/mlx5_flow.h index 10d700a..a3b82dd 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -222,7 +222,11 @@ 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. */
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counters *cs; /**< Holds the counters for the rule. */
> > +#else
> > struct ibv_counter_set *cs; /**< Holds the counters for the rule. */
> > +#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 05ab5fd..1c8bdba 100644
> > --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> > @@ -48,27 +48,32 @@
> > static struct mlx5_flow_counter *
> > flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared,
> > uint32_t id) {
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > struct priv *priv = dev->data->dev_private;
> > struct mlx5_flow_counter *cnt;
> >
> > - LIST_FOREACH(cnt, &priv->flow_counters, next) {
> > - if (!cnt->shared || cnt->shared != shared)
> > - continue;
> > - if (cnt->id != id)
> > - continue;
> > - cnt->ref_cnt++;
> > - return cnt;
> > + if (shared) {
> > + LIST_FOREACH(cnt, &priv->flow_counters, next)
> > + if (cnt->shared && cnt->id == id) {
> > + cnt->ref_cnt++;
> > + return cnt;
> > + }
> > }
> > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> >
> > struct mlx5_flow_counter tmpl = {
> > .shared = shared,
> > .id = id,
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + .cs = mlx5_glue->create_counter_set
> > + (priv->ctx,
> > + &(struct ibv_counters_init_attr){0}), #else
> > .cs = mlx5_glue->create_counter_set
> > (priv->ctx,
> > &(struct ibv_counter_set_init_attr){
> > .counter_set_id = id,
> > }),
> > +#endif
> > .hits = 0,
> > .bytes = 0,
> > };
> > @@ -77,17 +82,40 @@
> > rte_errno = errno;
> > return NULL;
> > }
> > +
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counter_attach_attr attach_attr = {0};
> > + int ret;
> > +
> > + attach_attr.counter_desc = IBV_COUNTER_PACKETS;
> > + attach_attr.index = 0;
> > + ret = ibv_attach_counters_point_flow(tmpl.cs, &attach_attr, NULL);
> > + if (!ret) {
> > + attach_attr.counter_desc = IBV_COUNTER_BYTES;
> > + attach_attr.index = 1;
> > + ret = ibv_attach_counters_point_flow(tmpl.cs,
> > + &attach_attr,
> > + NULL);
> > + }
> > + if (ret) {
> > + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
> > + rte_errno = ret;
> > + return NULL;
> > + }
> > +#endif
> > 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
> > +#else
> > rte_errno = ENOTSUP;
> > return NULL;
> > +#endif
> > }
> >
> > /**
> > @@ -947,7 +975,7 @@
> > flow->counter = flow_verbs_counter_new(dev, count-
> >shared,
> > count->id);
> > if (!flow->counter)
> > - return rte_flow_error_set(error, ENOTSUP,
> > + return rte_flow_error_set(error, rte_errno,
> >
> RTE_FLOW_ERROR_TYPE_ACTION,
> > action,
> > "cannot get counter"
> > @@ -955,7 +983,11 @@
> > }
> > *action_flags |= MLX5_ACTION_COUNT;
> > #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + counter.counters = flow->counter->cs; #else
> > counter.counter_set_handle = flow->counter->cs->handle;
> > +#endif
> > flow_verbs_spec_add(dev_flow, &counter, size); #endif
> > return 0;
> > diff --git a/drivers/net/mlx5/mlx5_glue.c
> > b/drivers/net/mlx5/mlx5_glue.c index 48590df..785234c 100644
> > --- a/drivers/net/mlx5/mlx5_glue.c
> > +++ b/drivers/net/mlx5/mlx5_glue.c
> > @@ -211,6 +211,39 @@
> > return ibv_dereg_mr(mr);
> > }
> >
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > +static struct ibv_counters *
> > +mlx5_glue_create_counters(struct ibv_context *context,
> > + struct ibv_counters_init_attr *init_attr) {
> > + return ibv_create_counters(context, init_attr); }
> > +
> > +static int
> > +mlx5_glue_destroy_counters(struct ibv_counters *counters) {
> > + return ibv_destroy_counters(counters); }
> > +
> > +static int
> > +mlx5_glue_attach_counters(struct ibv_counters *counters,
> > + struct ibv_counter_attach_attr *attr,
> > + struct ibv_flow *flow)
> > +{
> > + return ibv_attach_counters_point_flow(counters, attr, flow); }
> > +
> > +static int
> > +mlx5_glue_query_counters(struct ibv_counters *counters,
> > + uint64_t *counters_value,
> > + uint32_t ncounters,
> > + uint32_t flags)
> > +{
> > + return ibv_read_counters(counters, counters_value, ncounters,
> > +flags); } #endif
> > +
> > +#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > static struct ibv_counter_set *
> > mlx5_glue_create_counter_set(struct ibv_context *context,
> > struct ibv_counter_set_init_attr *init_attr) @@ -
> 262,6 +295,7
> > @@
> > return ibv_query_counter_set(query_attr, cs_data); #endif }
> > +#endif
> >
> > static void
> > mlx5_glue_ack_async_event(struct ibv_async_event *event) @@ -420,10
> > +454,17 @@
> > .modify_qp = mlx5_glue_modify_qp,
> > .reg_mr = mlx5_glue_reg_mr,
> > .dereg_mr = mlx5_glue_dereg_mr,
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + .create_counter_set = mlx5_glue_create_counters,
> > + .destroy_counter_set = mlx5_glue_destroy_counters,
> > + .attach_counter_set = mlx5_glue_attach_counters,
> > + .query_counter_set = mlx5_glue_query_counters, #else
> > .create_counter_set = mlx5_glue_create_counter_set,
> > .destroy_counter_set = mlx5_glue_destroy_counter_set,
> > .describe_counter_set = mlx5_glue_describe_counter_set,
> > .query_counter_set = mlx5_glue_query_counter_set,
> > +#endif
> > .ack_async_event = mlx5_glue_ack_async_event,
> > .get_async_event = mlx5_glue_get_async_event,
> > .port_state_str = mlx5_glue_port_state_str, diff --git
> > a/drivers/net/mlx5/mlx5_glue.h b/drivers/net/mlx5/mlx5_glue.h index
> > f6e4e38..504d487 100644
> > --- a/drivers/net/mlx5/mlx5_glue.h
> > +++ b/drivers/net/mlx5/mlx5_glue.h
> > @@ -96,6 +96,21 @@ struct mlx5_glue {
> > struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr,
> > size_t length, int access);
> > int (*dereg_mr)(struct ibv_mr *mr);
> > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
> > + struct ibv_counters *(*create_counter_set)
> > + (struct ibv_context *context,
> > + struct ibv_counters_init_attr *init_attr);
> > + int (*destroy_counter_set)(struct ibv_counters *cs);
> > + int (*attach_counter_set)
> > + (struct ibv_counters *cs,
> > + struct ibv_counter_attach_attr *attr,
> > + struct ibv_flow *flow);
> > + int (*query_counter_set)
> > + (struct ibv_counters *cs,
> > + uint64_t *counters_value,
> > + uint32_t ncounters,
> > + uint32_t flags);
> > +#else
> > struct ibv_counter_set *(*create_counter_set)
> > (struct ibv_context *context,
> > struct ibv_counter_set_init_attr *init_attr); @@ -106,6
> +121,7 @@
> > struct mlx5_glue {
> > struct ibv_counter_set_description *cs_desc);
> > int (*query_counter_set)(struct ibv_query_counter_set_attr
> *query_attr,
> > struct ibv_counter_set_data *cs_data);
> > +#endif
> > void (*ack_async_event)(struct ibv_async_event *event);
> > int (*get_async_event)(struct ibv_context *context,
> > struct ibv_async_event *event);
> > --
> > 1.8.3.1
> >
Thursday, October 11, 2018 5:52 PM, Slava Ovsiienko:
> Subject: RE: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> Linux-rdma v19 base
>
> > -----Original Message-----
> > From: Yongseok Koh
> > Sent: Thursday, October 4, 2018 2:48
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> > Linux-rdma v19 base
> >
> > On Wed, Oct 03, 2018 at 03:29:12PM +0000, Slava Ovsiienko wrote:
> > > Mellanox mlx5 PMD supports Flow Counters via Verbs library.
> > > The current implementation is based on the Mellanox proprietary
> > > Verbs library included in MLNX OFED packages. The Flow Counter
> > > support is recently added into linux-rdma release (v19), so the mlx5
> > > PMD update is needed to provide Counter feature on the base of linux-
> rdma.
> > >
> > > mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+ and
> > provide
> > > flow counters for both.
> > >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > > drivers/net/mlx5/Makefile | 10 ++++++++
> > > drivers/net/mlx5/mlx5.c | 6 +++++
> > > drivers/net/mlx5/mlx5_flow.c | 9 ++++++-
> > > drivers/net/mlx5/mlx5_flow.h | 4 +++
> > > drivers/net/mlx5/mlx5_flow_verbs.c | 52
> > ++++++++++++++++++++++++++++++--------
> > > drivers/net/mlx5/mlx5_glue.c | 41
> > ++++++++++++++++++++++++++++++
> > > drivers/net/mlx5/mlx5_glue.h | 16 ++++++++++++
> > > 7 files changed, 127 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> > > index ca1de9f..e3d2156 100644
> > > --- a/drivers/net/mlx5/Makefile
> > > +++ b/drivers/net/mlx5/Makefile
> > > @@ -162,6 +162,16 @@ mlx5_autoconf.h.new:
> > $(RTE_SDK)/buildtools/auto-config-h.sh
> > > type 'struct ibv_counter_set_init_attr' \
> > > $(AUTOCONF_OUTPUT)
> > > $Q sh -- '$<' '$@' \
> > > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> > > + infiniband/verbs.h \
> > > + type 'struct ibv_counters_init_attr' \
> > > + $(AUTOCONF_OUTPUT)
> > > + $Q sh -- '$<' '$@' \
> > > + HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
> > > + infiniband/verbs.h \
> > > + type 'struct ibv_counters_init_attr' \
> > > + $(AUTOCONF_OUTPUT)
> > > + $Q sh -- '$<' '$@' \
> >
> > I still don't understand what is different between the two. These are
> > exactly same checking, then why do you need to have two different
> > macros? From this script, HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is
> same
> > as HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, isn't it?
>
> We have three options:
> - no counter support in kernel at all
> - "old" counter support (ibv_counter_set_init_attr is defined in verbs.h)
> - "new" counter support (ibv_counters_init_attr is defined in verbs.h)
>
> Three options require at least two compilations flags. The meanings are
> chosen:
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT means there is counter
> support (of any type)
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 differentiates the
> support type
>
> This approach allows to avoid clumsy constructions in code like this:
> #if __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> || __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45)
This is not needed anyway.
The "old counters" exists only in MLNX_OFED not upstream.
Once the "new counters" were implemented upstream the next OFED completely replaced the old implementation.
Meaning, there is no driver having them both.
Hence, we can avoid this complex construction and just hold a single macro flag for the new counters.
@@ -162,6 +162,16 @@ mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
type 'struct ibv_counter_set_init_attr' \
$(AUTOCONF_OUTPUT)
$Q sh -- '$<' '$@' \
+ HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
+ infiniband/verbs.h \
+ type 'struct ibv_counters_init_attr' \
+ $(AUTOCONF_OUTPUT)
+ $Q sh -- '$<' '$@' \
+ HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
+ infiniband/verbs.h \
+ type 'struct ibv_counters_init_attr' \
+ $(AUTOCONF_OUTPUT)
+ $Q sh -- '$<' '$@' \
HAVE_RDMA_NL_NLDEV \
rdma/rdma_netlink.h \
enum RDMA_NL_NLDEV \
@@ -739,8 +739,10 @@
unsigned int mprq_min_stride_num_n = 0;
unsigned int mprq_max_stride_num_n = 0;
#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
+#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
struct ibv_counter_set_description cs_desc = { .counter_type = 0 };
#endif
+#endif
struct ether_addr mac;
char name[RTE_ETH_NAME_MAX_LEN];
int own_domain_id = 0;
@@ -1009,11 +1011,15 @@
DRV_LOG(DEBUG, "checksum offloading is %ssupported",
(config.hw_csum ? "" : "not "));
#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
+#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
config.flow_counter_en = !!attr.max_counter_sets;
mlx5_glue->describe_counter_set(ctx, 0, &cs_desc);
DRV_LOG(DEBUG, "counter type = %d, num of cs = %ld, attributes = %d",
cs_desc.counter_type, cs_desc.num_of_cs,
cs_desc.attributes);
+#else
+ config.flow_counter_en = 1;
+#endif
#endif
config.ind_table_max_size =
attr.rss_caps.max_rwq_indirection_table_size;
@@ -2306,6 +2306,13 @@ struct rte_flow *
if (flow->actions & MLX5_ACTION_COUNT) {
struct rte_flow_query_count *qc = data;
uint64_t counters[2] = {0, 0};
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ int err = mlx5_glue->query_counter_set(
+ flow->counter->cs,
+ counters,
+ RTE_DIM(counters),
+ IBV_READ_COUNTERS_ATTR_PREFER_CACHED);
+#else
struct ibv_query_counter_set_attr query_cs_attr = {
.cs = flow->counter->cs,
.query_flags = IBV_COUNTER_SET_FORCE_UPDATE,
@@ -2316,7 +2323,7 @@ struct rte_flow *
};
int err = mlx5_glue->query_counter_set(&query_cs_attr,
&query_out);
-
+#endif
if (err)
return rte_flow_error_set
(error, err,
@@ -222,7 +222,11 @@ 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. */
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ struct ibv_counters *cs; /**< Holds the counters for the rule. */
+#else
struct ibv_counter_set *cs; /**< Holds the counters for the rule. */
+#endif
uint64_t hits; /**< Number of packets matched by the rule. */
uint64_t bytes; /**< Number of bytes matched by the rule. */
};
@@ -48,27 +48,32 @@
static struct mlx5_flow_counter *
flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id)
{
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
struct priv *priv = dev->data->dev_private;
struct mlx5_flow_counter *cnt;
- LIST_FOREACH(cnt, &priv->flow_counters, next) {
- if (!cnt->shared || cnt->shared != shared)
- continue;
- if (cnt->id != id)
- continue;
- cnt->ref_cnt++;
- return cnt;
+ if (shared) {
+ LIST_FOREACH(cnt, &priv->flow_counters, next)
+ if (cnt->shared && cnt->id == id) {
+ cnt->ref_cnt++;
+ return cnt;
+ }
}
-#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
struct mlx5_flow_counter tmpl = {
.shared = shared,
.id = id,
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ .cs = mlx5_glue->create_counter_set
+ (priv->ctx,
+ &(struct ibv_counters_init_attr){0}),
+#else
.cs = mlx5_glue->create_counter_set
(priv->ctx,
&(struct ibv_counter_set_init_attr){
.counter_set_id = id,
}),
+#endif
.hits = 0,
.bytes = 0,
};
@@ -77,17 +82,40 @@
rte_errno = errno;
return NULL;
}
+
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ struct ibv_counter_attach_attr attach_attr = {0};
+ int ret;
+
+ attach_attr.counter_desc = IBV_COUNTER_PACKETS;
+ attach_attr.index = 0;
+ ret = ibv_attach_counters_point_flow(tmpl.cs, &attach_attr, NULL);
+ if (!ret) {
+ attach_attr.counter_desc = IBV_COUNTER_BYTES;
+ attach_attr.index = 1;
+ ret = ibv_attach_counters_point_flow(tmpl.cs,
+ &attach_attr,
+ NULL);
+ }
+ if (ret) {
+ claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs));
+ rte_errno = ret;
+ return NULL;
+ }
+#endif
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
+#else
rte_errno = ENOTSUP;
return NULL;
+#endif
}
/**
@@ -947,7 +975,7 @@
flow->counter = flow_verbs_counter_new(dev, count->shared,
count->id);
if (!flow->counter)
- return rte_flow_error_set(error, ENOTSUP,
+ return rte_flow_error_set(error, rte_errno,
RTE_FLOW_ERROR_TYPE_ACTION,
action,
"cannot get counter"
@@ -955,7 +983,11 @@
}
*action_flags |= MLX5_ACTION_COUNT;
#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ counter.counters = flow->counter->cs;
+#else
counter.counter_set_handle = flow->counter->cs->handle;
+#endif
flow_verbs_spec_add(dev_flow, &counter, size);
#endif
return 0;
@@ -211,6 +211,39 @@
return ibv_dereg_mr(mr);
}
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+static struct ibv_counters *
+mlx5_glue_create_counters(struct ibv_context *context,
+ struct ibv_counters_init_attr *init_attr)
+{
+ return ibv_create_counters(context, init_attr);
+}
+
+static int
+mlx5_glue_destroy_counters(struct ibv_counters *counters)
+{
+ return ibv_destroy_counters(counters);
+}
+
+static int
+mlx5_glue_attach_counters(struct ibv_counters *counters,
+ struct ibv_counter_attach_attr *attr,
+ struct ibv_flow *flow)
+{
+ return ibv_attach_counters_point_flow(counters, attr, flow);
+}
+
+static int
+mlx5_glue_query_counters(struct ibv_counters *counters,
+ uint64_t *counters_value,
+ uint32_t ncounters,
+ uint32_t flags)
+{
+ return ibv_read_counters(counters, counters_value, ncounters, flags);
+}
+#endif
+
+#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
static struct ibv_counter_set *
mlx5_glue_create_counter_set(struct ibv_context *context,
struct ibv_counter_set_init_attr *init_attr)
@@ -262,6 +295,7 @@
return ibv_query_counter_set(query_attr, cs_data);
#endif
}
+#endif
static void
mlx5_glue_ack_async_event(struct ibv_async_event *event)
@@ -420,10 +454,17 @@
.modify_qp = mlx5_glue_modify_qp,
.reg_mr = mlx5_glue_reg_mr,
.dereg_mr = mlx5_glue_dereg_mr,
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ .create_counter_set = mlx5_glue_create_counters,
+ .destroy_counter_set = mlx5_glue_destroy_counters,
+ .attach_counter_set = mlx5_glue_attach_counters,
+ .query_counter_set = mlx5_glue_query_counters,
+#else
.create_counter_set = mlx5_glue_create_counter_set,
.destroy_counter_set = mlx5_glue_destroy_counter_set,
.describe_counter_set = mlx5_glue_describe_counter_set,
.query_counter_set = mlx5_glue_query_counter_set,
+#endif
.ack_async_event = mlx5_glue_ack_async_event,
.get_async_event = mlx5_glue_get_async_event,
.port_state_str = mlx5_glue_port_state_str,
@@ -96,6 +96,21 @@ struct mlx5_glue {
struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr,
size_t length, int access);
int (*dereg_mr)(struct ibv_mr *mr);
+#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45
+ struct ibv_counters *(*create_counter_set)
+ (struct ibv_context *context,
+ struct ibv_counters_init_attr *init_attr);
+ int (*destroy_counter_set)(struct ibv_counters *cs);
+ int (*attach_counter_set)
+ (struct ibv_counters *cs,
+ struct ibv_counter_attach_attr *attr,
+ struct ibv_flow *flow);
+ int (*query_counter_set)
+ (struct ibv_counters *cs,
+ uint64_t *counters_value,
+ uint32_t ncounters,
+ uint32_t flags);
+#else
struct ibv_counter_set *(*create_counter_set)
(struct ibv_context *context,
struct ibv_counter_set_init_attr *init_attr);
@@ -106,6 +121,7 @@ struct mlx5_glue {
struct ibv_counter_set_description *cs_desc);
int (*query_counter_set)(struct ibv_query_counter_set_attr *query_attr,
struct ibv_counter_set_data *cs_data);
+#endif
void (*ack_async_event)(struct ibv_async_event *event);
int (*get_async_event)(struct ibv_context *context,
struct ibv_async_event *event);