[PATCH rdma-core 0/5] Enable mlx5 vendor functionality

[PATCH rdma-core 0/5] Enable mlx5 vendor functionality

[Yishai Hadas]

This patch set from Bodong is the supplementary part of the kernel series that
was accepted few months ago into 4.10.

The below functionality was accepted in mlx5 kernel driver in the vendor
channel path as some driver specific capabilities and functionality, as such it
is exposed to user applications in the same manner by using the direct access
mechanism.

The vendor specific capabilities are reported via the direct mlx5 query
capabilities API.

In addition, an option is added to provide vendor private data when creating a
CQ.  This option drops the need to extend the verbs layer each time with some
non-generic fields when creating a CQ. It adds an option to set some opaque
vendor data in some generic way which can fit any vendor, further details are
available in the commit log itself.

Specifically:
Report whether mlx5 hardware supports multi packet send WQE offload for RAW QP.
- Multi packet WQE enables sending multiple fix sized packets using a single
  WQE. When the WQE is completed a single CQE will be generated.

Report CQE compressing capabilities and enable creating a CQ with such
capabilities.
- CQE compressing reduces PCI overhead by coalescing and compressing multiple
  CQEs into a single merged CQE. Successful compressing improves message rate
  especially for small packet traffic. 

The series is exposed from 5 patches:
#1:   Pre-patch which aligns mlx5 query device response to 64 bit.
#2-3: Report mlx5 specific hardware capabilities through direct verbs.
#4:   Extend ibv_create_cq_ex to get some vendor opaque data.
#5:   Add support in mlx5 provider to create a CQ with compressed CQEs using
      the vendor data.

Pull request was sent:
https://github.com/linux-rdma/rdma-core/pull/90

Bodong Wang (5):
  mlx5: Explicitly align mlx5 query device response to 64 bit
  mlx5: Report multi packet send WQE capability for mlx5 based hardware
  mlx5: Report CQE compression capabilities through mlx5 direct verbs
  verbs: Add an option to provide vendor private data when creating a CQ
  mlx5: Add support to create a CQ with compressed CQEs

 libibverbs/man/ibv_create_cq_ex.3        |  2 ++
 libibverbs/verbs.h                       |  5 +++-
 providers/mlx5/man/mlx5dv_query_device.3 |  1 +
 providers/mlx5/mlx5-abi.h                | 15 ++++++++++--
 providers/mlx5/mlx5.c                    | 17 +++++++++++--
 providers/mlx5/mlx5.h                    |  6 +++++
 providers/mlx5/mlx5dv.h                  | 30 ++++++++++++++++++++++-
 providers/mlx5/verbs.c                   | 42 ++++++++++++++++++++++++++++++--
 8 files changed, 110 insertions(+), 8 deletions(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 1/5] mlx5: Explicitly align mlx5 query device response to 64 bit

[Yishai Hadas]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Explicitly align packet pacing response to 64 bit so that further
capabilities that going to be read will match to mlx5 kernel response.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5-abi.h | 7 ++++++-
 providers/mlx5/verbs.c    | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index 5d10486..8025131 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -263,6 +263,11 @@ struct mlx5_rss_caps {
 	__u8 reserved[7];
 };
 
+struct mlx5_packet_pacing_caps {
+	struct ibv_packet_pacing_caps caps;
+	__u32  reserved;
+};
+
 struct mlx5_query_device_ex_resp {
 	struct ibv_query_device_resp_ex ibv_resp;
 	__u32				comp_mask;
@@ -270,7 +275,7 @@ struct mlx5_query_device_ex_resp {
 	struct ibv_tso_caps		tso_caps;
 	struct mlx5_rss_caps            rss_caps; /* vendor data channel */
 	__u64				reserved_cqe_comp;
-	struct ibv_packet_pacing_caps	packet_pacing_caps;
+	struct mlx5_packet_pacing_caps	packet_pacing_caps;
 };
 
 #endif /* MLX5_ABI_H */
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index 98a9557..a659d3e 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1922,7 +1922,7 @@ int mlx5_query_device_ex(struct ibv_context *context,
 	attr->tso_caps = resp.tso_caps;
 	attr->rss_caps.rx_hash_fields_mask = resp.rss_caps.rx_hash_fields_mask;
 	attr->rss_caps.rx_hash_function = resp.rss_caps.rx_hash_function;
-	attr->packet_pacing_caps = resp.packet_pacing_caps;
+	attr->packet_pacing_caps = resp.packet_pacing_caps.caps;
 
 	major     = (raw_fw_ver >> 32) & 0xffff;
 	minor     = (raw_fw_ver >> 16) & 0xffff;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 2/5] mlx5: Report multi packet send WQE capability for mlx5 based hardware

[Yishai Hadas]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Report whether multi packet send WQE is supported or not through mlx5
direct verbs.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/man/mlx5dv_query_device.3 | 1 +
 providers/mlx5/mlx5-abi.h                | 2 ++
 providers/mlx5/mlx5.c                    | 8 +++++++-
 providers/mlx5/mlx5.h                    | 5 +++++
 providers/mlx5/mlx5dv.h                  | 3 ++-
 providers/mlx5/verbs.c                   | 3 +++
 6 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/providers/mlx5/man/mlx5dv_query_device.3 b/providers/mlx5/man/mlx5dv_query_device.3
index 1954714..b33a75b 100644
--- a/providers/mlx5/man/mlx5dv_query_device.3
+++ b/providers/mlx5/man/mlx5dv_query_device.3
@@ -35,6 +35,7 @@ enum mlx5dv_context_flags {
  * This flag indicates if CQE version 0 or 1 is needed.
  */
  MLX5DV_CONTEXT_FLAGS_CQE_V1 = (1 << 0),
+ MLX5DV_CONTEXT_FLAGS_MPW    = (1 << 1), /* Multi packet WQE is supported or not */
 .in -8
 };
 .fi
diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index 8025131..9a484b3 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -276,6 +276,8 @@ struct mlx5_query_device_ex_resp {
 	struct mlx5_rss_caps            rss_caps; /* vendor data channel */
 	__u64				reserved_cqe_comp;
 	struct mlx5_packet_pacing_caps	packet_pacing_caps;
+	__u32				support_multi_pkt_send_wqe;
+	__u32				reserved;
 };
 
 #endif /* MLX5_ABI_H */
diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index 87c85df..b7d737d 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -612,13 +612,18 @@ static int mlx5_map_internal_clock(struct mlx5_device *mdev,
 int mlx5dv_query_device(struct ibv_context *ctx_in,
 			 struct mlx5dv_context *attrs_out)
 {
+	struct mlx5_context *mctx = to_mctx(ctx_in);
+
 	attrs_out->comp_mask = 0;
 	attrs_out->version   = 0;
 	attrs_out->flags     = 0;
 
-	if (to_mctx(ctx_in)->cqe_version == MLX5_CQE_VERSION_V1)
+	if (mctx->cqe_version == MLX5_CQE_VERSION_V1)
 		attrs_out->flags |= MLX5DV_CONTEXT_FLAGS_CQE_V1;
 
+	if (mctx->vendor_cap_flags & MLX5_VENDOR_CAP_FLAGS_MPW)
+		attrs_out->flags |= MLX5DV_CONTEXT_FLAGS_MPW;
+
 	return 0;
 }
 
@@ -827,6 +832,7 @@ static int mlx5_init_context(struct verbs_device *vdev,
 	}
 
 	context->cmds_supp_uhw = resp.cmds_supp_uhw;
+	context->vendor_cap_flags = 0;
 
 	pthread_mutex_init(&context->qp_table_mutex, NULL);
 	pthread_mutex_init(&context->srq_table_mutex, NULL);
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index 57ed1a1..86f2438 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -180,6 +180,10 @@ enum {
 	MLX5_USER_CMDS_SUPP_UHW_CREATE_AH    = 1 << 1,
 };
 
+enum mlx5_vendor_cap_flags {
+	MLX5_VENDOR_CAP_FLAGS_MPW		= 1 << 0,
+};
+
 struct mlx5_resource {
 	enum mlx5_rsc_type	type;
 	uint32_t		rsn;
@@ -258,6 +262,7 @@ struct mlx5_context {
 	struct ibv_tso_caps		cached_tso_caps;
 	int				cmds_supp_uhw;
 	uint32_t			uar_size;
+	uint64_t			vendor_cap_flags; /* Use enum mlx5_vendor_cap_flags */
 };
 
 struct mlx5_bitmap {
diff --git a/providers/mlx5/mlx5dv.h b/providers/mlx5/mlx5dv.h
index 174b3f1..a04583d 100644
--- a/providers/mlx5/mlx5dv.h
+++ b/providers/mlx5/mlx5dv.h
@@ -71,7 +71,8 @@ enum mlx5dv_context_flags {
 	/*
 	 * This flag indicates if CQE version 0 or 1 is needed.
 	 */
-	MLX5DV_CONTEXT_FLAGS_CQE_V1 = (1 << 0),
+	MLX5DV_CONTEXT_FLAGS_CQE_V1	= (1 << 0),
+	MLX5DV_CONTEXT_FLAGS_MPW	= (1 << 1),
 };
 
 /*
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index a659d3e..ff9220d 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1924,6 +1924,9 @@ int mlx5_query_device_ex(struct ibv_context *context,
 	attr->rss_caps.rx_hash_function = resp.rss_caps.rx_hash_function;
 	attr->packet_pacing_caps = resp.packet_pacing_caps.caps;
 
+	if (resp.support_multi_pkt_send_wqe)
+		mctx->vendor_cap_flags |= MLX5_VENDOR_CAP_FLAGS_MPW;
+
 	major     = (raw_fw_ver >> 32) & 0xffff;
 	minor     = (raw_fw_ver >> 16) & 0xffff;
 	sub_minor = raw_fw_ver & 0xffff;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 3/5] mlx5: Report CQE compression capabilities through mlx5 direct verbs

[Yishai Hadas]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Report max number of compressed and aggregated CQEs in a single
merged CQE and the formats of mini CQE supported for responder.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5-abi.h |  3 ++-
 providers/mlx5/mlx5.c     |  9 ++++++++-
 providers/mlx5/mlx5.h     |  1 +
 providers/mlx5/mlx5dv.h   | 17 +++++++++++++++++
 providers/mlx5/verbs.c    |  2 ++
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index 9a484b3..b07067b 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -34,6 +34,7 @@
 #define MLX5_ABI_H
 
 #include <infiniband/kern-abi.h>
+#include "mlx5dv.h"
 
 #define MLX5_UVERBS_MIN_ABI_VERSION	1
 #define MLX5_UVERBS_MAX_ABI_VERSION	1
@@ -274,7 +275,7 @@ struct mlx5_query_device_ex_resp {
 	__u32				response_length;
 	struct ibv_tso_caps		tso_caps;
 	struct mlx5_rss_caps            rss_caps; /* vendor data channel */
-	__u64				reserved_cqe_comp;
+	struct mlx5dv_cqe_comp_caps	cqe_comp_caps;
 	struct mlx5_packet_pacing_caps	packet_pacing_caps;
 	__u32				support_multi_pkt_send_wqe;
 	__u32				reserved;
diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index b7d737d..3c32e59 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -613,8 +613,8 @@ int mlx5dv_query_device(struct ibv_context *ctx_in,
 			 struct mlx5dv_context *attrs_out)
 {
 	struct mlx5_context *mctx = to_mctx(ctx_in);
+	uint64_t comp_mask_out = 0;
 
-	attrs_out->comp_mask = 0;
 	attrs_out->version   = 0;
 	attrs_out->flags     = 0;
 
@@ -624,6 +624,13 @@ int mlx5dv_query_device(struct ibv_context *ctx_in,
 	if (mctx->vendor_cap_flags & MLX5_VENDOR_CAP_FLAGS_MPW)
 		attrs_out->flags |= MLX5DV_CONTEXT_FLAGS_MPW;
 
+	if (attrs_out->comp_mask & MLX5DV_CONTEXT_MASK_CQE_COMPRESION) {
+		attrs_out->cqe_comp_caps = mctx->cqe_comp_caps;
+		comp_mask_out |= MLX5DV_CONTEXT_MASK_CQE_COMPRESION;
+	}
+
+	attrs_out->comp_mask = comp_mask_out;
+
 	return 0;
 }
 
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index 86f2438..bb383d4 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -263,6 +263,7 @@ struct mlx5_context {
 	int				cmds_supp_uhw;
 	uint32_t			uar_size;
 	uint64_t			vendor_cap_flags; /* Use enum mlx5_vendor_cap_flags */
+	struct mlx5dv_cqe_comp_caps	cqe_comp_caps;
 };
 
 struct mlx5_bitmap {
diff --git a/providers/mlx5/mlx5dv.h b/providers/mlx5/mlx5dv.h
index a04583d..9f30e6e 100644
--- a/providers/mlx5/mlx5dv.h
+++ b/providers/mlx5/mlx5dv.h
@@ -58,6 +58,16 @@ enum {
 	MLX5_SND_DBR	= 1,
 };
 
+enum mlx5dv_context_comp_mask {
+	MLX5DV_CONTEXT_MASK_CQE_COMPRESION	= 1 << 0,
+	MLX5DV_CONTEXT_MASK_RESERVED		= 1 << 1,
+};
+
+struct mlx5dv_cqe_comp_caps {
+	uint32_t max_num;
+	uint32_t supported_format; /* enum mlx5dv_cqe_comp_res_format */
+};
+
 /*
  * Direct verbs device-specific attributes
  */
@@ -65,6 +75,7 @@ struct mlx5dv_context {
 	uint8_t		version;
 	uint64_t	flags;
 	uint64_t	comp_mask;
+	struct mlx5dv_cqe_comp_caps	cqe_comp_caps;
 };
 
 enum mlx5dv_context_flags {
@@ -281,6 +292,12 @@ struct mlx5_cqe64 {
 	uint8_t		op_own;
 };
 
+enum mlx5dv_cqe_comp_res_format {
+	MLX5DV_CQE_RES_FORMAT_HASH		= 1 << 0,
+	MLX5DV_CQE_RES_FORMAT_CSUM		= 1 << 1,
+	MLX5DV_CQE_RES_FORMAT_RESERVED		= 1 << 2,
+};
+
 static MLX5DV_ALWAYS_INLINE
 uint8_t mlx5dv_get_cqe_owner(struct mlx5_cqe64 *cqe)
 {
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index ff9220d..0e02772 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1927,6 +1927,8 @@ int mlx5_query_device_ex(struct ibv_context *context,
 	if (resp.support_multi_pkt_send_wqe)
 		mctx->vendor_cap_flags |= MLX5_VENDOR_CAP_FLAGS_MPW;
 
+	mctx->cqe_comp_caps = resp.cqe_comp_caps;
+
 	major     = (raw_fw_ver >> 32) & 0xffff;
 	minor     = (raw_fw_ver >> 16) & 0xffff;
 	sub_minor = raw_fw_ver & 0xffff;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Yishai Hadas]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch drops the need to extend the verbs layer each time with
some non generic fields when creating a CQ. It adds an option to set
some opaque vendor data in some generic way which can fit any vendor.

The solution introduced for creating a CQ but expects to fit for
other IB objects as of QP, SRQ, etc.

It comes as a pre-patch for supplying some private mlx5 fields in
downstream patches.

The solution:
1) Introduce a new comp_mask bit (i.e. IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA)
   and a matching void* vendor data field.

2) Vendor should expose its header file for direct access with
   relevant private structures.

3) Application should supply the input based on vendor-defined structure.

4) Upon getting the private data the vendor will cast to its internal
   structure and use the data as part of CQ creation.

Note:
The format of the input is vendor specific and should be defined in an
extended way to support backward/forward compatibility. (e.g. by using
comp_mask, etc.)

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/man/ibv_create_cq_ex.3 | 2 ++
 libibverbs/verbs.h                | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libibverbs/man/ibv_create_cq_ex.3 b/libibverbs/man/ibv_create_cq_ex.3
index a6ae769..2a1bc92 100644
--- a/libibverbs/man/ibv_create_cq_ex.3
+++ b/libibverbs/man/ibv_create_cq_ex.3
@@ -28,6 +28,7 @@ struct ibv_comp_channel *channel;          /* Completion channel where completio
 int                     comp_vector;       /* Completion vector used to signal completion events. Must be >= 0 and < context->num_comp_vectors. */
 uint64_t                wc_flags;          /* The wc_flags that should be returned in ibv_poll_cq_ex. Or'ed bit of enum ibv_wc_flags_ex. */
 uint32_t                comp_mask;         /* compatibility mask (extended verb). */
+void                    *vendor_data;      /* Vendor private data for creating a CQ. */
 .in -8
 };
 
@@ -44,6 +45,7 @@ enum ibv_wc_flags_ex {
 
 enum ibv_cq_init_attr_mask {
         IBV_CQ_INIT_ATTR_MASK_FLAGS             = 1 << 0,
+	IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA       = 1 << 1,
 };
 
 enum ibv_create_cq_attr_flags {
diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index 25f4ede..2575e01 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -1399,7 +1399,8 @@ struct ibv_context {
 
 enum ibv_cq_init_attr_mask {
 	IBV_CQ_INIT_ATTR_MASK_FLAGS	= 1 << 0,
-	IBV_CQ_INIT_ATTR_MASK_RESERVED	= 1 << 1
+	IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA = 1 << 1,
+	IBV_CQ_INIT_ATTR_MASK_RESERVED	= 1 << 2
 };
 
 enum ibv_create_cq_attr_flags {
@@ -1430,6 +1431,8 @@ struct ibv_cq_init_attr_ex {
 	 * enum ibv_create_cq_attr_flags
 	 */
 	uint32_t		flags;
+	/* Vendor private data for creating a CQ */
+	void			*vendor_data;
 };
 
 enum ibv_values_mask {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Jason Gunthorpe]

On Mon, Mar 06, 2017 at 04:06:43PM +0200, Yishai Hadas wrote:
> From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> This patch drops the need to extend the verbs layer each time with
> some non generic fields when creating a CQ. It adds an option to set
> some opaque vendor data in some generic way which can fit any vendor.
> 
> The solution introduced for creating a CQ but expects to fit for
> other IB objects as of QP, SRQ, etc.
> 
> It comes as a pre-patch for supplying some private mlx5 fields in
> downstream patches.
> 
> The solution:
> 1) Introduce a new comp_mask bit (i.e. IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA)
>    and a matching void* vendor data field.
> 
> 2) Vendor should expose its header file for direct access with
>    relevant private structures.
> 
> 3) Application should supply the input based on vendor-defined structure.
> 
> 4) Upon getting the private data the vendor will cast to its internal
>    structure and use the data as part of CQ creation.

I'm not sure I like this, why not create a mlx5dv_create_cq?

struct ibv_cq_ex *mlx5dv_create_cq_ex(struct mlx5dv_context *context,
                                   struct ibv_cq_init_attr_ex *cq_attr,
				   struct mlx5_.... *vendor_data)

Seems much more type safe to me.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Yishai Hadas]

On 3/6/2017 8:07 PM, Jason Gunthorpe wrote:
> On Mon, Mar 06, 2017 at 04:06:43PM +0200, Yishai Hadas wrote:
>> From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> This patch drops the need to extend the verbs layer each time with
>> some non generic fields when creating a CQ. It adds an option to set
>> some opaque vendor data in some generic way which can fit any vendor.
>>
>> The solution introduced for creating a CQ but expects to fit for
>> other IB objects as of QP, SRQ, etc.
>>
>> It comes as a pre-patch for supplying some private mlx5 fields in
>> downstream patches.
>>
>> The solution:
>> 1) Introduce a new comp_mask bit (i.e. IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA)
>>    and a matching void* vendor data field.
>>
>> 2) Vendor should expose its header file for direct access with
>>    relevant private structures.
>>
>> 3) Application should supply the input based on vendor-defined structure.
>>
>> 4) Upon getting the private data the vendor will cast to its internal
>>    structure and use the data as part of CQ creation.
>
> I'm not sure I like this, why not create a mlx5dv_create_cq?
>

Below notes justify the suggested scheme comparing adding 
mlx5dv_create_cq() API for that purpose:

1) From developer point of view it's fully preferred to use single API 
(e.g. ibv_create_cq_ex) otherwise developer should choose, do I use 
ibv_create_cq_ex() or mlx5dv_create_cq(). If I use the latter, do I need 
mlx5dv_destroy_cq() or can I use ibv_destroy_cq()...

2) We would like to support this scheme for other calls as well and 
enjoy libibverbs generic services on top, that's can be achieved by the 
suggested scheme.

3) Other vendors can use the suggested scheme without the need to expose 
any xxx_create_cq() API, all that is needed is just to expose direct H 
file, this can be done also by mlx5 customers who don't want to use the 
mlx5_dv direct calls but to enjoy from the option to supply vendor data 
as part of CQ creation.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Jason Gunthorpe]

On Tue, Mar 07, 2017 at 07:18:40PM +0200, Yishai Hadas wrote:

> >I'm not sure I like this, why not create a mlx5dv_create_cq?
> >
> 
> Below notes justify the suggested scheme comparing adding mlx5dv_create_cq()
> API for that purpose:
> 
> 1) From developer point of view it's fully preferred to use single API (e.g.
> ibv_create_cq_ex) otherwise developer should choose, do I use

Why? That isn't right, from a developer point of view it is better to
have strong type safety so the API is obvious how to use correctly.

How is the developer to know to pass some random mlxdv struct in the
void *? How do they know when that is even safe to do?

> ibv_create_cq_ex() or mlx5dv_create_cq(). If I use the latter, do I need
> mlx5dv_destroy_cq() or can I use ibv_destroy_cq()...

Since ibv_create_cq_ex would return a ibv_cq, it obviously would be
destroyed via ibv_destroy_cq

> 2) We would like to support this scheme for other calls as well and enjoy
> libibverbs generic services on top, that's can be achieved by the suggested
> scheme.

Other calls can be evaluated when we get to them, but I can't see why
they would be any better..

> 3) Other vendors can use the suggested scheme without the need to expose any
> xxx_create_cq() API, all that is needed is just to expose direct H file,
> this can be done also by mlx5 customers who don't want to use the mlx5_dv
> direct calls but to enjoy from the option to supply vendor data as part of
> CQ creation.

I don't think this is a positive.

Using the dv interface should be obvious and *always* create a
link-time dependency on the driver. If someone doesn't want to do that
then they shouldn't use the interface.

This makes it very clear to the user that they are touching a
non-portable API and they need to plan accordingly.

We have caused ourselves alot of pain by trying to be too clever with
function pointers in the past. That totally defeats the usual symver
mechanism for compatability and should *NOT* be done by this dv stuff.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Yishai Hadas]

On 3/7/2017 8:17 PM, Jason Gunthorpe wrote:
> On Tue, Mar 07, 2017 at 07:18:40PM +0200, Yishai Hadas wrote:
>
>>> I'm not sure I like this, why not create a mlx5dv_create_cq?
>>>
>>
>> Below notes justify the suggested scheme comparing adding mlx5dv_create_cq()
>> API for that purpose:
>>
>> 1) From developer point of view it's fully preferred to use single API (e.g.
>> ibv_create_cq_ex) otherwise developer should choose, do I use
>
> Why? That isn't right, from a developer point of view it is better to
> have strong type safety so the API is obvious how to use correctly.

Having 2 APIs for similar purpose usually doesn't make sense and might 
bring confusion and code duplication. The mlx5dv_create_cq() from 
application point of view can be used to create a regular CQ when no 
vendor data will be supplied, the idea is to have a single API for clarity.

> How is the developer to know to pass some random mlxdv struct in the
> void *? How do they know when that is even safe to do?

The developer who passes vendor data should compile his code with the 
vendor H file to have the input structure, this is true in both options.

The structure to be used is defined in the vendor H file and its name 
should match the API, in the mlx5 case it's named as 
mlx5dv_create_cq_vendor_data. An explicit comment can be added if it's 
not clear enough. This is the expected behavior for coming vendor 
structures in mlx5 and in any other vendors if will come.

Once the correct structure is used there should be *no* safety issue, 
please note that the vendor data defined to be as some extended 
structure which can only be grown based on comp_mask so that 
compatibility will be saved in any case.


> Other calls can be evaluated when we get to them, but I can't see why
> they would be any better..

Better think from day one on coming ones which may come soon(e.g. QP 
vendor data), the suggested scheme can fit to all objects and prevent 
the need to introduce more APIs each time.

>> 3) Other vendors can use the suggested scheme without the need to expose any
>> xxx_create_cq() API, all that is needed is just to expose direct H file,
>> this can be done also by mlx5 customers who don't want to use the mlx5_dv
>> direct calls but to enjoy from the option to supply vendor data as part of
>> CQ creation.
>
> I don't think this is a positive.

This is an option that can be easily adapted by other vendors.

> Using the dv interface should be obvious and *always* create a
> link-time dependency on the driver. If someone doesn't want to do that
> then they shouldn't use the interface.
>
> This makes it very clear to the user that they are touching a
> non-portable API and they need to plan accordingly.

This is correct for using DV APIs which are fully driver specific but 
its not a must to add an API which is quite similar to the ibv_xxx one.

> We have caused ourselves alot of pain by trying to be too clever with
> function pointers in the past. That totally defeats the usual symver
> mechanism for compatability and should *NOT* be done by this dv stuff.

That's why we want to prevent from new APIs when really not required.

I believe that we should get input from Doug and other people re the 
above discussion.

Doug,
Can we get please your input here ?

Thanks,
Yishai


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Jason Gunthorpe]

On Wed, Mar 08, 2017 at 12:10:18AM +0200, Yishai Hadas wrote:

> >>1) From developer point of view it's fully preferred to use single API (e.g.
> >>ibv_create_cq_ex) otherwise developer should choose, do I use
> >
> >Why? That isn't right, from a developer point of view it is better to
> >have strong type safety so the API is obvious how to use correctly.
> 
> Having 2 APIs for similar purpose usually doesn't make sense and might bring
> confusion and code duplication. The mlx5dv_create_cq() from application
> point of view can be used to create a regular CQ when no vendor data will be
> supplied, the idea is to have a single API for clarity.

It is very common to have 'basic' and 'complex' API entry points. I
don't see a problem here.

> Once the correct structure is used there should be *no* safety issue, please
> note that the vendor data defined to be as some extended structure which can
> only be grown based on comp_mask so that compatibility will be saved in any
> case.

No, you still have to make sure that your ib_device is somehow a mlx5.

mlx5dv_create_cq can directly verify this on entry, but ibv_create_cq
will assume the vendor data is for the device that is bound - this is
why it is inherently unsafe and unfriendly to discard the type
information via the void *.

> >This makes it very clear to the user that they are touching a
> >non-portable API and they need to plan accordingly.
> 
> This is correct for using DV APIs which are fully driver specific but its
> not a must to add an API which is quite similar to the ibv_xxx one.

When I suggested this approach at Plumbers, I explained that all
create methods should be the vendor specific call, and that they should
return a generic object that could be used with the other standard API
entry points.

The idea you want to pollute many of the existing common APIs with a
vendor_data argument is ugly as sin, the entire *point* of this
approach is to keep all that uglyness in *your* dv driver and make it
*very clear* to users that they are outside the standard and cannot
export portability when they touch this stuff.

I don't find your arguments against this idea very compelling..

> >We have caused ourselves alot of pain by trying to be too clever with
> >function pointers in the past. That totally defeats the usual symver
> >mechanism for compatability and should *NOT* be done by this dv stuff.
> 
> That's why we want to prevent from new APIs when really not required.

Symbol versions are well understood, we should use them sanely when
possible.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Yishai Hadas]

On 3/8/2017 12:22 AM, Jason Gunthorpe wrote:
> It is very common to have 'basic' and 'complex' API entry points. I
> don't see a problem here.

It's not an issue of 'basic' and 'complex' API but a question of code 
sharing and resource managing.

Using *one* API (i.e. ibv_create_cq_ex) shares the libibverbs code on 
the creation path. Look at current code at '__lib_ibv_create_cq_ex()', 
it manages locking, ref_counting and set the IB attributes, in the 
future it may do other shared stuff as well.

Creating common IB objects as of CQ/QP/etc. with some vendor specific 
API might end-up with code duplication, missing functionality or even 
might lead to some bugs as of out-of-sync with the main path creation
of libibverbs.

Think about future feature of resource tracking per context as of
handling recovery/error flow on its objects upon getting a device fatal 
event, collecting statistics on its open objects ,etc.
We expect a shared code for the above in few places in libibverbs. Being 
part of the standard flow should guarantee that any future feature will 
be added automatically without a special care from any vendor. As the 
underlay object is really an IB standard object it makes sense to have a 
single API for its creation.

>> Once the correct structure is used there should be *no* safety issue, please
>> note that the vendor data defined to be as some extended structure which can
>> only be grown based on comp_mask so that compatibility will be saved in any
>> case.
>
> No, you still have to make sure that your ib_device is somehow a mlx5.

We are talking on an application that previously read some vendor 
capabilities using a direct API (e.g. mlx5dv_query_device) to know 
whether the vendor functionality is supported.
As same ibv_context is used in both APIs (e.g. mlx5dv_query_device, 
ibv_create_cq_ex) it should be very clear to application's writer which 
vendor is used.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Jason Gunthorpe]

On Tue, Mar 14, 2017 at 05:14:55PM +0200, Yishai Hadas wrote:
> It's not an issue of 'basic' and 'complex' API but a question of code
> sharing and resource managing.

This can always be acheived with some code restructuring.

eg we can put the tail of __lib_ibv_create_cq_ex into a helper, or
have an internal create_cq that accepts the void *provider_data

There is a lot less code inside libibverbs that outside, I'd rather
have an obvious clean user API than make our lives a tiny bit easier.

> >>Once the correct structure is used there should be *no* safety
> >>issue, please note that the vendor data defined to be as some
> >>extended structure which can only be grown based on comp_mask so
> >>that compatibility will be saved in any case.
> >
> >No, you still have to make sure that your ib_device is somehow a mlx5.
> 
> We are talking on an application that previously read some vendor
> capabilities using a direct API (e.g. mlx5dv_query_device) to know whether
> the vendor functionality is supported.

With these patches if the app passes the wrong thing into the
vendor_data it crashes, with a mlx5_create_cq it would get an error
code. Error codes are better.

Yishai, even if we go this way, stop using the word 'vendor'. It is
not vendor data.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Hefty, Sean]

> That's why we want to prevent from new APIs when really not required.
> 
> I believe that we should get input from Doug and other people re the
> above discussion.
> 
> Doug,
> Can we get please your input here ?

I agree with Jason.  There are clear advantages to have type safe APIs.  The overloading that already occurs in the existing APIs (e.g. create/modify_qp, post_send) make them extremely difficult to use.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Add an option to provide vendor private data when creating a CQ

[Doug Ledford]

On Tue, 2017-03-07 at 11:17 -0700, Jason Gunthorpe wrote:
> Using the dv interface should be obvious and *always* create a
> link-time dependency on the driver. If someone doesn't want to do
> that
> then they shouldn't use the interface.
> 
> This makes it very clear to the user that they are touching a
> non-portable API and they need to plan accordingly.

As far as I'm concerned, this is probably the most important argument
made so far in this thread.  So, Yishai, to answer your question, I'm
in agreement with Jason and Sean.  The flexibility that you are so fond
of in the original patch submission sounds nice, but it leads to all
sorts of problems.  Better to have a more rigid, typed interface than
something that people can't understand how to use by looking at it.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 5/5] mlx5: Add support to create a CQ with compressed CQEs

[Yishai Hadas]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch uses the vendor data mechanism that was introduced as
part of ibv_create_cq_ex to create a CQ with some vendor specific
attributes. Specifically, it enables creating a CQ in a mode that
few CQEs may be compressed into a single CQE.

Usage:
- The driver exposes its create_cq vendor data structure to be filled
  by the application. (i.e. mlx5dv_create_cq_vendor_data)

- Upon CQ creation if IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA bit was set,
  cast to the above structure and make sure that its input is supported.

- If input is valid, pass it via the vendor channel to mlx5 kernel driver.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5-abi.h |  3 +++
 providers/mlx5/mlx5dv.h   | 10 ++++++++++
 providers/mlx5/verbs.c    | 35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index b07067b..cd1c876 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -118,6 +118,9 @@ struct mlx5_create_cq {
 	__u64				buf_addr;
 	__u64				db_addr;
 	__u32				cqe_size;
+	__u8				cqe_comp_en;
+	__u8				cqe_comp_res_format;
+	__u16				reserved;
 };
 
 struct mlx5_create_cq_resp {
diff --git a/providers/mlx5/mlx5dv.h b/providers/mlx5/mlx5dv.h
index 9f30e6e..fa53cff 100644
--- a/providers/mlx5/mlx5dv.h
+++ b/providers/mlx5/mlx5dv.h
@@ -292,12 +292,22 @@ struct mlx5_cqe64 {
 	uint8_t		op_own;
 };
 
+enum mlx5dv_create_cq_vendor_data_mask {
+	MLX5DV_CREATE_CQ_MASK_COMPRESSED_CQE	= 1 << 0,
+	MLX5DV_CREATE_CQ_MASK_RESERVED		= 1 << 1,
+};
+
 enum mlx5dv_cqe_comp_res_format {
 	MLX5DV_CQE_RES_FORMAT_HASH		= 1 << 0,
 	MLX5DV_CQE_RES_FORMAT_CSUM		= 1 << 1,
 	MLX5DV_CQE_RES_FORMAT_RESERVED		= 1 << 2,
 };
 
+struct mlx5dv_create_cq_vendor_data {
+	uint64_t comp_mask; /* Use enum mlx5dv_create_cq_vendor_data_mask */
+	uint8_t cqe_comp_res_format; /* Use enum mlx5dv_cqe_comp_res_format */
+};
+
 static MLX5DV_ALWAYS_INLINE
 uint8_t mlx5dv_get_cqe_owner(struct mlx5_cqe64 *cqe)
 {
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index 0e02772..289b318 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -332,7 +332,8 @@ enum {
 };
 
 enum {
-	CREATE_CQ_SUPPORTED_COMP_MASK = IBV_CQ_INIT_ATTR_MASK_FLAGS
+	CREATE_CQ_SUPPORTED_COMP_MASK = IBV_CQ_INIT_ATTR_MASK_FLAGS |
+					IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA
 };
 
 enum {
@@ -350,6 +351,8 @@ static struct ibv_cq_ex *create_cq(struct ibv_context *context,
 	int				ret;
 	int				ncqe;
 	FILE *fp = to_mctx(context)->dbg_fp;
+	struct mlx5dv_create_cq_vendor_data *vendor_data;
+	struct mlx5_context *mctx = to_mctx(context);
 
 	if (!cq_attr->cqe) {
 		mlx5_dbg(fp, MLX5_DBG_CQ, "CQE invalid\n");
@@ -428,6 +431,36 @@ static struct ibv_cq_ex *create_cq(struct ibv_context *context,
 	cmd.db_addr  = (uintptr_t) cq->dbrec;
 	cmd.cqe_size = cqe_sz;
 
+	if (cq_attr->comp_mask & IBV_CQ_INIT_ATTR_MASK_VENDOR_DATA) {
+		if (!(cq_attr->vendor_data)) {
+			mlx5_dbg(fp, MLX5_DBG_CQ, "Vendor data is requried\n");
+			errno = EINVAL;
+			goto err_db;
+		}
+
+		vendor_data = (struct mlx5dv_create_cq_vendor_data *)(cq_attr->vendor_data);
+
+		if (vendor_data->comp_mask & ~(MLX5DV_CREATE_CQ_MASK_RESERVED - 1)) {
+			mlx5_dbg(fp, MLX5_DBG_CQ,
+				 "Unsupported vendor comp_mask for create_cq\n");
+			errno = EINVAL;
+			goto err_db;
+		}
+
+		if (vendor_data->comp_mask & MLX5DV_CREATE_CQ_MASK_COMPRESSED_CQE) {
+			if (mctx->cqe_comp_caps.max_num &&
+			    (vendor_data->cqe_comp_res_format &
+			     mctx->cqe_comp_caps.supported_format)) {
+				cmd.cqe_comp_en = 1;
+				cmd.cqe_comp_res_format = vendor_data->cqe_comp_res_format;
+			} else {
+				mlx5_dbg(fp, MLX5_DBG_CQ, "CQE Compression is not supported\n");
+				errno = EINVAL;
+				goto err_db;
+			}
+		}
+	}
+
 	ret = ibv_cmd_create_cq(context, ncqe - 1, cq_attr->channel,
 				cq_attr->comp_vector,
 				ibv_cq_ex_to_cq(&cq->ibv_cq), &cmd.ibv_cmd,
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html