Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the Linux-rdma v19 base

[Slava Ovsiienko <viacheslavo@mellanox.com>]

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