[PATCH libibverbs 0/3] Add support for TCP segmentation offload (TSO)

[PATCH libibverbs 0/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

Hi Doug,

This patch set from Bodong exposes to user space applications an API to use the
TSO offload over verbs and let them enjoying from its benefits.

The relevant kernel patches regarding capabilities were already taken into your
tree for coming kernel of 4.8.

The series was tested successfully with mlx5 driver (lib, kernel) and can be
accessed also from my openfabrics GIT at:
git://openfabrics.org/~yishaih/libibverbs.git branch: tso

Will send shortly the candidate libmlx5 patch as some driver reference
code for that API.

Yishai

In general,
TSO enables an application to pass a packet with large chunk of data than the
MTU to the NIC and let it breaks into small packets as part of its send flow.
The TSO engine will split the packet into separate packets and insert the
relevant user specified L2/L3/L4 headers automatically.
This is a technique for increasing outbound throughput of high-bandwidth
network connections by reducing CPU overhead.

For some extra information about TSO, below link can be helpful:
https://en.wikipedia.org/wiki/Large_segment_offload

Verbs usage,
TSO traffic is performed by ibv_post_send with opcode IBV_WR_TSO.
The following information should be supplied within the associated send WR:
- A pointer to the packet header.
- Header size.
- The maximum segment size (mss) that hardware should generate in
  its TSO engine.

The above information required to send a TSO packet is similar to the IB kernel
API.  http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L1237

The TSO capabilities as of the supported QP types, max TSO payload can be
achieved upon query device.
 
When creating a TSO eligible QP, user should supply the maximum TSO header size
in order to let providers preparing their SQ buffer accordingly. This need is
similar to other capabilities that are given as part of QP creation for that
purpose.

Bodong Wang (3):
  Fix ibv_cmd_query_device_ex to return valid output
  Add support for TCP segmentation offload (TSO)
  Update man pages for TSO support

 examples/devinfo.c         | 20 ++++++++++++++++++++
 include/infiniband/verbs.h | 33 +++++++++++++++++++++++++--------
 man/ibv_create_qp_ex.3     |  1 +
 man/ibv_post_send.3        | 36 ++++++++++++++++++++++++------------
 man/ibv_query_device_ex.3  | 22 ++++++++++++++--------
 src/cmd.c                  |  9 +--------
 6 files changed, 85 insertions(+), 36 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 libibverbs 1/3] Fix ibv_cmd_query_device_ex to return valid output

[Yishai Hadas]

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

Make sure to return valid output by memset the extended fields to zero.
No need to deal with specific fields any more.

Currently, extended fields will be assigned to some value or 0 depending
on response from the command. When adding a new extended field, relevant
variables must be cleared if no response got from the kernel.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 src/cmd.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/cmd.c b/src/cmd.c
index cb9e34c..4b3304f 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -163,6 +163,7 @@ int ibv_cmd_query_device_ex(struct ibv_context *context,
 	cmd->comp_mask = 0;
 	cmd->reserved = 0;
 	memset(attr->orig_attr.fw_ver, 0, sizeof(attr->orig_attr.fw_ver));
+	memset(&attr->comp_mask, 0, attr_size - sizeof(attr->orig_attr));
 	err = write(context->cmd_fd, cmd, cmd_size);
 	if (err != cmd_size)
 		return errno;
@@ -184,8 +185,6 @@ int ibv_cmd_query_device_ex(struct ibv_context *context,
 				resp->odp_caps.per_transport_caps.uc_odp_caps;
 			attr->odp_caps.per_transport_caps.ud_odp_caps =
 				resp->odp_caps.per_transport_caps.ud_odp_caps;
-		} else {
-			memset(&attr->odp_caps, 0, sizeof(attr->odp_caps));
 		}
 	}
 
@@ -196,8 +195,6 @@ int ibv_cmd_query_device_ex(struct ibv_context *context,
 		    offsetof(struct ibv_query_device_resp_ex, timestamp_mask) +
 		    sizeof(resp->timestamp_mask))
 			attr->completion_timestamp_mask = resp->timestamp_mask;
-		else
-			attr->completion_timestamp_mask = 0;
 	}
 
 	if (attr_size >= offsetof(struct ibv_device_attr_ex, hca_core_clock) +
@@ -206,8 +203,6 @@ int ibv_cmd_query_device_ex(struct ibv_context *context,
 		    offsetof(struct ibv_query_device_resp_ex, hca_core_clock) +
 		    sizeof(resp->hca_core_clock))
 			attr->hca_core_clock = resp->hca_core_clock;
-		else
-			attr->hca_core_clock = 0;
 	}
 
 	if (attr_size >= offsetof(struct ibv_device_attr_ex, device_cap_flags_ex) +
@@ -216,8 +211,6 @@ int ibv_cmd_query_device_ex(struct ibv_context *context,
 		    offsetof(struct ibv_query_device_resp_ex, device_cap_flags_ex) +
 		    sizeof(resp->device_cap_flags_ex))
 			attr->device_cap_flags_ex = resp->device_cap_flags_ex;
-		else
-			attr->device_cap_flags_ex = 0;
 	}
 
 	return 0;
-- 
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

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

1) Add a structure to define a TSO packet as part of struct ibv_send_wr.
2) Add IBV_WR_TSO opcode to be used as part of post_send.
3) Add IBV_WC_TSO to be used as part of poll_cq to report a TSO completion.
4) Add IBV_QP_INIT_ATTR_MAX_TSO_HEADER to define the maximum TSO header size
   when creating a QP. This is needed to let providers prepare their SQ buffer
   to fit application's usage.
5) Report TSO capabilities when querying a device.

In order to preserve the size of ibv_send_wr structure and prevents some
performance penalty, the TSO definition was added under a union with the
memory window stuff, those options are mutual exclusive.

The TSO definition should include:
- A pointer to the packet header.
- Header size.
- The maximum segment size (mss) that the hardware should generate in
  its TSO engine.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 examples/devinfo.c         | 20 ++++++++++++++++++++
 include/infiniband/verbs.h | 33 +++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/examples/devinfo.c b/examples/devinfo.c
index 915ebb3..c497650 100644
--- a/examples/devinfo.c
+++ b/examples/devinfo.c
@@ -345,6 +345,25 @@ static void print_device_cap_flags_ex(uint64_t device_cap_flags_ex)
 		       ex_flags & unknown_flags);
 }
 
+static void print_tso_caps(const struct ibv_tso_caps *caps)
+{
+	uint32_t unknown_general_caps = ~(1 << IBV_QPT_RAW_PACKET |
+					  1 << IBV_QPT_UD);
+	printf("\ttso_caps:\n");
+	printf("\tmax_tso:\t\t\t%d\n", caps->max_tso);
+
+	if (caps->max_tso) {
+		printf("\tsupported_qp:\n");
+		if (ibv_is_qpt_supported(caps->supported_qpts, IBV_QPT_RAW_PACKET))
+			printf("\t\t\t\t\tSUPPORT_RAW_PACKET\n");
+		if (ibv_is_qpt_supported(caps->supported_qpts, IBV_QPT_UD))
+			printf("\t\t\t\t\tSUPPORT_UD\n");
+		if (caps->supported_qpts & unknown_general_caps)
+			printf("\t\t\t\t\tUnknown flags: 0x%" PRIX32 "\n",
+			       caps->supported_qpts & unknown_general_caps);
+	}
+}
+
 static int print_hca_cap(struct ibv_device *ib_dev, uint8_t ib_port)
 {
 	struct ibv_context *ctx;
@@ -445,6 +464,7 @@ static int print_hca_cap(struct ibv_device *ib_dev, uint8_t ib_port)
 
 		printf("\tdevice_cap_flags_ex:\t\t0x%" PRIX64 "\n", device_attr.device_cap_flags_ex);
 		print_device_cap_flags_ex(device_attr.device_cap_flags_ex);
+		print_tso_caps(&device_attr.tso_caps);
 	}
 
 	for (port = 1; port <= device_attr.orig_attr.phys_port_cnt; ++port) {
diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
index ccf1d51..87e4332 100644
--- a/include/infiniband/verbs.h
+++ b/include/infiniband/verbs.h
@@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
 	IBV_ODP_SUPPORT = 1 << 0,
 };
 
+#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
+
+struct ibv_tso_caps {
+	uint32_t max_tso;
+	uint32_t supported_qpts;
+};
+
 struct ibv_device_attr_ex {
 	struct ibv_device_attr	orig_attr;
 	uint32_t		comp_mask;
@@ -216,6 +223,7 @@ struct ibv_device_attr_ex {
 	uint64_t		completion_timestamp_mask;
 	uint64_t		hca_core_clock;
 	uint64_t		device_cap_flags_ex;
+	struct ibv_tso_caps	tso_caps;
 };
 
 enum ibv_mtu {
@@ -357,6 +365,7 @@ enum ibv_wc_opcode {
 	IBV_WC_FETCH_ADD,
 	IBV_WC_BIND_MW,
 	IBV_WC_LOCAL_INV,
+	IBV_WC_TSO,
 /*
  * Set value of IBV_WC_RECV so consumers can test if a completion is a
  * receive by testing (opcode & IBV_WC_RECV).
@@ -636,7 +645,8 @@ enum ibv_qp_init_attr_mask {
 	IBV_QP_INIT_ATTR_PD		= 1 << 0,
 	IBV_QP_INIT_ATTR_XRCD		= 1 << 1,
 	IBV_QP_INIT_ATTR_CREATE_FLAGS	= 1 << 2,
-	IBV_QP_INIT_ATTR_RESERVED	= 1 << 3
+	IBV_QP_INIT_ATTR_MAX_TSO_HEADER = 1 << 3,
+	IBV_QP_INIT_ATTR_RESERVED	= 1 << 4
 };
 
 enum ibv_qp_create_flags {
@@ -657,7 +667,7 @@ struct ibv_qp_init_attr_ex {
 	struct ibv_pd	       *pd;
 	struct ibv_xrcd	       *xrcd;
 	uint32_t                create_flags;
-
+	uint16_t		max_tso_header;
 };
 
 enum ibv_qp_open_attr_mask {
@@ -756,6 +766,7 @@ enum ibv_wr_opcode {
 	IBV_WR_LOCAL_INV,
 	IBV_WR_BIND_MW,
 	IBV_WR_SEND_WITH_INV,
+	IBV_WR_TSO,
 };
 
 enum ibv_send_flags {
@@ -802,12 +813,18 @@ struct ibv_send_wr {
 			uint32_t    remote_srqn;
 		} xrc;
 	} qp_type;
-	struct {
-		struct ibv_mw	*mw;
-		uint32_t		rkey;
-		struct ibv_mw_bind_info	bind_info;
-	} bind_mw;
-
+	union {
+		struct {
+			struct ibv_mw	*mw;
+			uint32_t		rkey;
+			struct ibv_mw_bind_info	bind_info;
+		} bind_mw;
+		struct {
+			void		       *hdr;
+			uint16_t		hdr_sz;
+			uint16_t		mss;
+		} tso;
+	};
 };
 
 struct ibv_recv_wr {
-- 
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 libibverbs 3/3] Update man pages for TSO support

[Yishai Hadas]

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

Update relevant man pages for TSO:
- Add max_tso_header as part of ibv_create_qp_ex man page.
- Add IBV_WR_TSO opcode and update send operation support table for
  RAW_PACKET QP as part of ibv_post_send man page.
- Add TSO capabilities as part of ibv_query_device_ex man page.

In addition, fixed a typo as part of updating ibv_post_send related to
ibv_odp_caps.


Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 man/ibv_create_qp_ex.3    |  1 +
 man/ibv_post_send.3       | 36 ++++++++++++++++++++++++------------
 man/ibv_query_device_ex.3 | 22 ++++++++++++++--------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/man/ibv_create_qp_ex.3 b/man/ibv_create_qp_ex.3
index f772a57..a2a67a4 100644
--- a/man/ibv_create_qp_ex.3
+++ b/man/ibv_create_qp_ex.3
@@ -34,6 +34,7 @@ uint32_t                comp_mask;	/* Identifies valid fields */
 struct ibv_pd          *pd;		/* PD to be associated with the QP */
 struct ibv_xrcd        *xrcd;		/* XRC domain to be associated with the target QP */
 enum ibv_qp_create_flags create_flags;	/* Creation flags for this QP */
+uint16_t                max_tso_header; /* Maximum TSO header size */
 .in -8
 };
 .sp
diff --git a/man/ibv_post_send.3 b/man/ibv_post_send.3
index f918afb..0d3fa66 100644
--- a/man/ibv_post_send.3
+++ b/man/ibv_post_send.3
@@ -69,6 +69,8 @@ uint32_t remote_srqn;            /* Number of the remote SRQ */
 } xrc;
 .in -8
 } qp_type;
+union {
+.in +8
 struct {
 .in +8
 struct ibv_mw            *mw;             /* Memory window (MW) of type 2 to bind */
@@ -76,6 +78,15 @@ uint32_t                 rkey;            /* The desired new rkey of the MW */
 struct ibv_mw_bind_info  bind_info;       /* MW additional bind information */
 .in -8
 } bind_mw;
+struct {
+.in +8
+void			*hdr;	/* Pointer address of inline header */
+uint16_t		hdr_sz;	/* Inline header size */
+uint16_t		mss;	/* Maximum segment size for each TSO fragment */
+.in -8
+} tso;
+.in -8
+};
 .in -8
 };
 .fi
@@ -104,18 +115,19 @@ uint32_t                lkey;                   /* Key of the local Memory Regio
 Each QP Transport Service Type supports a specific set of opcodes, as shown in the following table:
 .PP
 .nf
-OPCODE                      | IBV_QPT_UD | IBV_QPT_UC | IBV_QPT_RC | IBV_QPT_XRC_SEND
-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-
-IBV_WR_SEND                 |     X      |     X      |     X     |     X
-IBV_WR_SEND_WITH_IMM        |     X      |     X      |     X     |     X
-IBV_WR_RDMA_WRITE           |            |     X      |     X     |     X
-IBV_WR_RDMA_WRITE_WITH_IMM  |            |     X      |     X     |     X
-IBV_WR_RDMA_READ            |            |            |     X     |     X
-IBV_WR_ATOMIC_CMP_AND_SWP   |            |            |     X     |     X
-IBV_WR_ATOMIC_FETCH_AND_ADD |            |            |     X     |     X
-IBV_WR_LOCAL_INV            |            |     X      |     X     |     X
-IBV_WR_BIND_MW              |            |     X      |     X     |     X
-IBV_WR_SEND_WITH_INV        |            |     X      |     X     |     X
+OPCODE                      | IBV_QPT_UD | IBV_QPT_UC | IBV_QPT_RC | IBV_QPT_XRC_SEND | IBV_QPT_RAW_PACKET
+\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-+\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-
+IBV_WR_SEND                 |     X      |     X      |     X      |         X        |         X
+IBV_WR_SEND_WITH_IMM        |     X      |     X      |     X      |         X        |
+IBV_WR_RDMA_WRITE           |            |     X      |     X      |         X        |
+IBV_WR_RDMA_WRITE_WITH_IMM  |            |     X      |     X      |         X        |
+IBV_WR_RDMA_READ            |            |            |     X      |         X        |
+IBV_WR_ATOMIC_CMP_AND_SWP   |            |            |     X      |         X        |
+IBV_WR_ATOMIC_FETCH_AND_ADD |            |            |     X      |         X        |
+IBV_WR_LOCAL_INV            |            |     X      |     X      |         X        |
+IBV_WR_BIND_MW              |            |     X      |     X      |         X        |
+IBV_WR_SEND_WITH_INV        |            |     X      |     X      |         X        |
+IBV_WR_TSO                  |            |            |            |                  |         X
 .fi
 .PP
 The attribute send_flags describes the properties of the \s-1WR\s0. It is either 0 or the bitwise \s-1OR\s0 of one or more of the following flags:
diff --git a/man/ibv_query_device_ex.3 b/man/ibv_query_device_ex.3
index 5097be0..d06b40f 100644
--- a/man/ibv_query_device_ex.3
+++ b/man/ibv_query_device_ex.3
@@ -26,17 +26,18 @@ uint32_t               comp_mask;                  /* Compatibility mask that de
 struct ibv_odp_caps    odp_caps;                   /* On-Demand Paging capabilities */
 uint64_t               completion_timestamp_mask;  /* Completion timestamp mask (0 = unsupported) */
 uint64_t               hca_core_clock;             /* The frequency (in kHZ) of the HCA (0 = unsupported) */
-uint64_t               device_cap_flags_ex;    /* Extended device capability flags */
+uint64_t               device_cap_flags_ex;        /* Extended device capability flags */
+struct ibv_tso_caps    tso_caps;                   /* TCP segmentation offload capabilities */
 .in -8
 };
 
-struct ibv_exp_odp_caps {
-	uint64_t	general_odp_caps;  /* Mask with enum ibv_odp_general_cap_bits */
-	struct {
-		uint32_t	rc_odp_caps;      /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
-		uint32_t	uc_odp_caps;      /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
-		uint32_t	ud_odp_caps;      /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
-	} per_transport_caps;
+struct ibv_odp_caps {
+        uint64_t general_odp_caps;    /* Mask with enum ibv_odp_general_cap_bits */
+        struct {
+                uint32_t rc_odp_caps; /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
+                uint32_t uc_odp_caps; /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
+                uint32_t ud_odp_caps; /* Mask with enum ibv_odp_tranport_cap_bits to know which operations are supported. */
+        } per_transport_caps;
 };
 
 enum ibv_odp_general_cap_bits {
@@ -51,6 +52,11 @@ enum ibv_odp_transport_cap_bits {
         IBV_ODP_SUPPORT_ATOMIC   = 1 << 4, /* RDMA-Atomic operations support on-demand paging */
 };
 
+struct ibv_tso_caps {
+        uint32_t max_tso;        /* Maximum payload size in bytes supported for segmentation by TSO engine.*/
+        uint32_t supported_qpts; /* Bitmap showing which QP types are supported by TSO operation. */
+};
+
 .fi
 .SH "RETURN VALUE"
 .B ibv_query_device_ex()
-- 
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 libibverbs 1/3] Fix ibv_cmd_query_device_ex to return valid output

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Sagi Grimberg]

> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
> index ccf1d51..87e4332 100644
> --- a/include/infiniband/verbs.h
> +++ b/include/infiniband/verbs.h
> @@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
>  	IBV_ODP_SUPPORT = 1 << 0,
>  };
>
> +#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))

This looks useful, but not directly related to this patch. Would
be better to add it in a stand-alone patch (even if its a one-liner).

Otherwise looks good,

Reviewed-by: Sagi Grimberg <sag-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
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 libibverbs 3/3] Update man pages for TSO support

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
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 libibverbs 0/3] Add support for TCP segmentation offload (TSO)

[Sagi Grimberg]

> Hi Doug,
>
> This patch set from Bodong exposes to user space applications an API to use the
> TSO offload over verbs and let them enjoying from its benefits.
>
> The relevant kernel patches regarding capabilities were already taken into your
> tree for coming kernel of 4.8.
>
> The series was tested successfully with mlx5 driver (lib, kernel) and can be
> accessed also from my openfabrics GIT at:
> git://openfabrics.org/~yishaih/libibverbs.git branch: tso
>
> Will send shortly the candidate libmlx5 patch as some driver reference
> code for that API.

Any updates on this set? Would love to see it in your tree Doug!
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Bodong Wang]

On 8/17/2016 9:35 AM, Sagi Grimberg wrote:
>> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
>> index ccf1d51..87e4332 100644
>> --- a/include/infiniband/verbs.h
>> +++ b/include/infiniband/verbs.h
>> @@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
>>  	IBV_ODP_SUPPORT = 1 << 0,
>>  };
>>
>> +#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
> This looks useful, but not directly related to this patch. Would
> be better to add it in a stand-alone patch (even if its a one-liner).
>
> Otherwise looks good,
>
> Reviewed-by: Sagi Grimberg <sag-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>
Hi Sagi,

Thanks for reviewing. This macro will be used by the TSO to check the
capabilities(devinfo.c), so it's actually related to this patch.

Best Regards,

Bodong

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

On 8/17/2016 5:35 PM, Sagi Grimberg wrote:
>
>> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
>> index ccf1d51..87e4332 100644
>> --- a/include/infiniband/verbs.h
>> +++ b/include/infiniband/verbs.h
>> @@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
>>      IBV_ODP_SUPPORT = 1 << 0,
>>  };
>>
>> +#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
>
> This looks useful, but not directly related to this patch. Would
> be better to add it in a stand-alone patch (even if its a one-liner).

It's used internally by that patch, look at print_tso_caps. Better stay 
with that.

> Otherwise looks good,
>
> Reviewed-by: Sagi Grimberg <sag-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

Thanks Sagi for your review.

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On 7/28/2016 3:19 PM, Yishai Hadas wrote:
> 3) Add IBV_WC_TSO to be used as part of poll_cq to report a TSO completion.

why is that needed? what's the proposed/possible applicative usage of 
that bit?

Or.

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On 7/28/2016 3:19 PM, Yishai Hadas wrote:
> +		struct {
> +			void		       *hdr;
> +			uint16_t		hdr_sz;
> +			uint16_t		mss;
> +		} tso;

can't this be placed within the struct ibv_send_wr :: wr union ?

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

On 8/18/2016 9:11 AM, Or Gerlitz wrote:
> On 7/28/2016 3:19 PM, Yishai Hadas wrote:
>> +        struct {
>> +            void               *hdr;
>> +            uint16_t        hdr_sz;
>> +            uint16_t        mss;
>> +        } tso;
>
> can't this be placed within the struct ibv_send_wr :: wr union ?

No, union 'wr' includes 'ud' which may be used potentially with TSO.
This is not the case for bind_mw, see commit message as well.

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

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

> From: Or Gerlitz
> Sent: Thursday, August 18, 2016 9:06 AM
> To: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Bodong Wang
> <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Tal Alon
> <talal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH libibverbs 2/3] Add support for TCP segmentation
> offload (TSO)
> 
> On 7/28/2016 3:19 PM, Yishai Hadas wrote:
> > 3) Add IBV_WC_TSO to be used as part of poll_cq to report a TSO
> completion.
> 
> why is that needed? what's the proposed/possible applicative usage of
> that bit?

It's consistent with the kernel API that adds similar bit for LSO.
http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L842

Specifically, it can give an application an accurate  knowledge about the completion type
as done for other cases as part of ibv_post_send as of IBV_WC_BIND_MW,
IBV_WC_LOCAL_INV, etc.



--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On 8/18/2016 10:55 AM, Yishai Hadas wrote:
>> 3) Add IBV_WC_TSO to be used as part of poll_cq to report a TSO completion.
>>
>> why is that needed? what's the proposed/possible applicative usage of
>> that bit?
> It's consistent with the kernel API that adds similar bit for LSO.
> http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L842

C&P is not something you want to use as argument, specifically if no one 
uses this 10y old define nowadays (and maybe never)

net.git]# git grep IB_WC_LSO drivers/infiniband/ include/rdma/
drivers/infiniband/hw/mlx4/cq.c: wc->opcode    = IB_WC_LSO;
include/rdma/ib_verbs.h:        IB_WC_LSO,


> Specifically, it can give an application an accurate  knowledge about the completion type as done for other cases as part of ibv_post_send as of IBV_WC_BIND_MW, IBV_WC_LOCAL_INV, etc.

IPoIB uses LSO for 10y and doesn't need that, I don't see why user-space 
IPoIBs would need that


--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Yishai Hadas]

On 8/18/2016 11:28 AM, Or Gerlitz wrote:
> On 8/18/2016 10:26 AM, Yishai Hadas wrote:
>> n 8/18/2016 9:11 AM, Or Gerlitz wrote:
>>> On 7/28/2016 3:19 PM, Yishai Hadas wrote:
>>>> +        struct {
>>>> +            void               *hdr;
>>>> +            uint16_t        hdr_sz;
>>>> +            uint16_t        mss;
>>>> +        } tso;
>>>
>>> can't this be placed within the struct ibv_send_wr :: wr union ?
>>
>> No, union 'wr' includes 'ud' which may be used potentially with TSO.
>> This is not the case for bind_mw, see commit message as well.
>
>
> FWIW, doing this as the below, AFAIU doesn't enlarge the wr union, thoughts?
>
> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
> index ccf1d51..6652750 100644
> --- a/include/infiniband/verbs.h
> +++ b/include/infiniband/verbs.h
> @@ -795,8 +795,17 @@ struct ibv_send_wr {
>                         struct ibv_ah  *ah;
>                         uint32_t        remote_qpn;
>                         uint32_t        remote_qkey;
> +                       void            *hdr;
> +                       uint16_t        hdr_sz;
> +                       uint16_t        mss;
>                 } ud;
> +               struct {
> +                       void            *hdr;
> +                       uint16_t        hdr_sz;
> +                       uint16_t        mss;
> +               } lso_raw;

No reason to duplicate, better put in one place as introduced by the patch.

>         } wr;
> +
>         union {
>                 struct {
>                         uint32_t    remote_srqn;
>

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On Thu, Aug 18, 2016 at 10:55 AM, Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> From: Or Gerlitz

>> why is that needed? what's the proposed/possible applicative usage of
>> that bit?
>
> It's consistent with the kernel API that adds similar bit for LSO.
> http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L842

C&P is not something you want to use as argument, specifically if no
one uses this 10y old define nowadays (and maybe never)

net.git]# git grep IB_WC_LSO drivers/infiniband/ include/rdma/
drivers/infiniband/hw/mlx4/cq.c: wc->opcode    = IB_WC_LSO;
include/rdma/ib_verbs.h:        IB_WC_LSO,

> Specifically, it can give an application an accurate  knowledge about the completion type
> as done for other cases as part of ibv_post_send as of IBV_WC_BIND_MW,
> IBV_WC_LOCAL_INV, etc.

IPoIB uses LSO for 10y and doesn't need that, I don't see why
user-space IPoIBs would need that
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Sagi Grimberg]

>>> why is that needed? what's the proposed/possible applicative usage of
>>> that bit?
>>
>> It's consistent with the kernel API that adds similar bit for LSO.
>> http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L842
>
> C&P is not something you want to use as argument, specifically if no
> one uses this 10y old define nowadays (and maybe never)
>
> net.git]# git grep IB_WC_LSO drivers/infiniband/ include/rdma/
> drivers/infiniband/hw/mlx4/cq.c: wc->opcode    = IB_WC_LSO;
> include/rdma/ib_verbs.h:        IB_WC_LSO,

IPoIB never looks at wc->opcode, does it mean that it is redundant?

I tend to agree that something that is not used shouldn't exist, but
my concern is what is the semantics in the lack of IBV_WC_TSO? You post
IBV_WR_TSO and when you get the completion and happen to look at the
wc->opcode, what would you expect to see?
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Jason Gunthorpe]

On Wed, Aug 17, 2016 at 05:35:42PM +0300, Sagi Grimberg wrote:
> 
> >diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
> >index ccf1d51..87e4332 100644
> >+++ b/include/infiniband/verbs.h
> >@@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
> > 	IBV_ODP_SUPPORT = 1 << 0,
> > };
> >
> >+#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
> 
> This looks useful, but not directly related to this patch. Would
> be better to add it in a stand-alone patch (even if its a one-liner).

And of course it should be a static inline....

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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Bodong Wang]

On 8/19/2016 1:33 PM, Jason Gunthorpe wrote:
> On Wed, Aug 17, 2016 at 05:35:42PM +0300, Sagi Grimberg wrote:
>>> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
>>> index ccf1d51..87e4332 100644
>>> +++ b/include/infiniband/verbs.h
>>> @@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
>>> 	IBV_ODP_SUPPORT = 1 << 0,
>>> };
>>>
>>> +#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
>> This looks useful, but not directly related to this patch. Would
>> be better to add it in a stand-alone patch (even if its a one-liner).
> And of course it should be a static inline....
>
> Jason
>
This is a macro, not a function.

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Bart Van Assche]

On 08/19/16 12:00, Bodong Wang wrote:
> On 8/19/2016 1:33 PM, Jason Gunthorpe wrote:
>> On Wed, Aug 17, 2016 at 05:35:42PM +0300, Sagi Grimberg wrote:
>>>> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
>>>> index ccf1d51..87e4332 100644
>>>> +++ b/include/infiniband/verbs.h
>>>> @@ -209,6 +209,13 @@ enum ibv_odp_general_caps {
>>>> 	IBV_ODP_SUPPORT = 1 << 0,
>>>> };
>>>>
>>>> +#define ibv_is_qpt_supported(caps, qpt) ((caps) & (1 << (qpt)))
>>> This looks useful, but not directly related to this patch. Would
>>> be better to add it in a stand-alone patch (even if its a one-liner).
>> And of course it should be a static inline....
>
> This is a macro, not a function.

Hello Bodong,

In case you didn't know this: when introducing new functionality, 
whenever there is a choice between introducing this functionality as a 
macro or an inline function, using an inline function is preferred. With 
an inline the compiler namely can perform type checking, arguments do 
not have to be surrounded by parentheses and using an argument multiple 
times in the body of the inline function is safe even if the argument 
expression has side effects.

Bart.

--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On Fri, Aug 19, 2016 at 12:26 PM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
>
>>>> why is that needed? what's the proposed/possible applicative usage of
>>>> that bit?
>>>
>>>
>>> It's consistent with the kernel API that adds similar bit for LSO.
>>> http://lxr.free-electrons.com/source/include/rdma/ib_verbs.h#L842
>>
>>
>> C&P is not something you want to use as argument, specifically if no
>> one uses this 10y old define nowadays (and maybe never)
>>
>> net.git]# git grep IB_WC_LSO drivers/infiniband/ include/rdma/
>> drivers/infiniband/hw/mlx4/cq.c: wc->opcode    = IB_WC_LSO;
>> include/rdma/ib_verbs.h:        IB_WC_LSO,
>
>
> IPoIB never looks at wc->opcode, does it mean that it is redundant?

I tend to agree :)

> I tend to agree that something that is not used shouldn't exist, but
> my concern is what is the semantics in the lack of IBV_WC_TSO? You post
> IBV_WR_TSO and when you get the completion and happen to look at the
> wc->opcode, what would you expect to see?

again, fact is that the application (ipoib) doesn't need it, as you
can see mlx5 even doesn't implement that.

The application should expect a completion of a send, that's it.

# git grep WR_LSO drivers/infiniband/
drivers/infiniband/hw/mlx4/qp.c:                        if (wr->opcode
== IB_WR_LSO) {
drivers/infiniband/hw/mlx5/qp.c:        if (wr->opcode == IB_WR_LSO) {
drivers/infiniband/sw/rxe/rxe_comp.c:   case IB_WR_LSO:
         return IB_WC_LSO;

drivers/infiniband/ulp/ipoib/ipoib_ib.c:
priv->tx_wr.wr.opcode   = IB_WR_LSO;

[root@r-dcs58 net.git]# git grep WC_LSO drivers/infiniband/
drivers/infiniband/hw/mlx4/cq.c:                        wc->opcode
= IB_WC_LSO;
drivers/infiniband/sw/rxe/rxe_comp.c:   case IB_WR_LSO:
         return IB_WC_LSO;
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Sagi Grimberg]

>> I tend to agree that something that is not used shouldn't exist, but
>> my concern is what is the semantics in the lack of IBV_WC_TSO? You post
>> IBV_WR_TSO and when you get the completion and happen to look at the
>> wc->opcode, what would you expect to see?
>
> again, fact is that the application (ipoib) doesn't need it, as you
> can see mlx5 even doesn't implement that.
>
> The application should expect a completion of a send, that's it.

It's sort of unprecedented to post IBV_WR_XXX and see IBV_WC_YYY but
I'm on the fence here, I guess we can go either way...
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On 8/21/2016 7:33 PM, Sagi Grimberg wrote:
>
>>> I tend to agree that something that is not used shouldn't exist, but
>>> my concern is what is the semantics in the lack of IBV_WC_TSO? You post
>>> IBV_WR_TSO and when you get the completion and happen to look at the
>>> wc->opcode, what would you expect to see?
>>
>> again, fact is that the application (ipoib) doesn't need it, as you
>> can see mlx5 even doesn't implement that.
>>
>> The application should expect a completion of a send, that's it.
>
> It's sort of unprecedented to post IBV_WR_XXX and see IBV_WC_YYY but

I am not sure, many applications avoid IBV_WC_YYY all together 
(selective signaling) for performance matters. Its fact of life that LL 
drivers and HWs must know that this is LSO WR, but that they have no 
special treatment for LSO CQE

> I'm on the fence here, I guess we can go either way...

so lets skip
--
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 libibverbs 2/3] Add support for TCP segmentation offload (TSO)

[Or Gerlitz]

On Sun, Aug 21, 2016 at 7:33 PM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:

> It's sort of unprecedented to post IBV_WR_XXX and see IBV_WC_YYY but

I am not sure, many applications avoid IBV_WC_YYY all together
(selective signaling) for performance matters. Its fact of life that
LL drivers and HWs must know that this is LSO WR, but that they have
no special treatment for LSO CQE

> I'm on the fence here, I guess we can go either way...

so lets skip
--
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