[PATCH rdma-core 0/3] Add packet pacing support

[PATCH rdma-core 0/3] Add packet pacing support

[Yishai Hadas]

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

Packet pacing is a feature to control packet injection rate in arbitrary rates.
One typical use case is for streaming vendors to control the bandwidth of
different customers based on service coverage.

Packet pacing is a rate limiting and shaping for a QP (SQ for RAW QP), set and
change the rate is done by modifying QP.

This series made the following high level changes:
1. Report rate limit capabilities, it includes:
   - The maximum and minimum rate limit in kbps supported
     by packet pacing.
   - Bitmap showing which QP types are supported by packet pacing operation.
2. Use the extended uverbs command for modify QP to add rate limit support.
3. Add support in mlx5 provider.

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

Yishai

Bodong Wang (3):
  ibverbs: Report packet pacing capabilities when querying device
  ibverbs: Add support for packet pacing
  mlx5: Add packet pacing support

 libibverbs/cmd.c                     | 55 ++++++++++++++++++++++++++++++++----
 libibverbs/driver.h                  |  5 ++++
 libibverbs/examples/devinfo.c        | 21 ++++++++++++++
 libibverbs/kern-abi.h                | 27 +++++++++++++++---
 libibverbs/libibverbs.map            |  5 ++++
 libibverbs/man/ibv_modify_qp.3       |  2 ++
 libibverbs/man/ibv_query_device_ex.3 |  7 +++++
 libibverbs/verbs.h                   | 11 +++++++-
 providers/mlx5/mlx5-abi.h            |  2 ++
 providers/mlx5/verbs.c               | 20 +++++++++++--
 10 files changed, 143 insertions(+), 12 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/3] ibverbs: Report packet pacing capabilities when querying device

[Yishai Hadas]

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

The capabilities include two parts:
1. The minimum (qp_rate_limit_min) and maximum(qp_rate_limit_max)
   rate limit in kbps.
2. QP types supported by the device (supported_qpts).

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/examples/devinfo.c        | 21 +++++++++++++++++++++
 libibverbs/man/ibv_query_device_ex.3 |  7 +++++++
 libibverbs/verbs.h                   |  7 +++++++
 3 files changed, 35 insertions(+)

diff --git a/libibverbs/examples/devinfo.c b/libibverbs/examples/devinfo.c
index 78e92b9..54976bb 100644
--- a/libibverbs/examples/devinfo.c
+++ b/libibverbs/examples/devinfo.c
@@ -384,6 +384,26 @@ static void print_rss_caps(const struct ibv_rss_caps *caps)
 	}
 }
 
+static void print_packet_pacing_caps(const struct ibv_packet_pacing_caps *caps)
+{
+	uint32_t unknown_general_caps = ~(1 << IBV_QPT_RAW_PACKET |
+					  1 << IBV_QPT_UD);
+	printf("\tpacket_pacing_caps:\n");
+	printf("\t\tqp_rate_limit_min:\t%ukbps\n", caps->qp_rate_limit_min);
+	printf("\t\tqp_rate_limit_max:\t%ukbps\n", caps->qp_rate_limit_max);
+
+	if (caps->qp_rate_limit_max) {
+		printf("\t\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;
@@ -487,6 +507,7 @@ static int print_hca_cap(struct ibv_device *ib_dev, uint8_t ib_port)
 		print_tso_caps(&device_attr.tso_caps);
 		print_rss_caps(&device_attr.rss_caps);
 		printf("\tmax_wq_type_rq:\t\t\t%u\n", device_attr.max_wq_type_rq);
+		print_packet_pacing_caps(&device_attr.packet_pacing_caps);
 	}
 
 	for (port = 1; port <= device_attr.orig_attr.phys_port_cnt; ++port) {
diff --git a/libibverbs/man/ibv_query_device_ex.3 b/libibverbs/man/ibv_query_device_ex.3
index 4da63a0..c291017 100644
--- a/libibverbs/man/ibv_query_device_ex.3
+++ b/libibverbs/man/ibv_query_device_ex.3
@@ -31,6 +31,7 @@ uint64_t               device_cap_flags_ex;        /* Extended device capability
 struct ibv_tso_caps    tso_caps;                   /* TCP segmentation offload capabilities */
 struct ibv_rss_caps    rss_caps;                   /* RSS capabilities */
 uint32_t               max_wq_type_rq;             /* Max Work Queue from type RQ */
+struct ibv_packet_pacing_caps packet_pacing_caps; /* Packet pacing capabilities */
 .in -8
 };
 
@@ -68,6 +69,12 @@ struct ibv_rss_caps {
         uint8_t  rx_hash_function;                 /* Mask with enum ibv_rx_hash_function_flags to know which hash functions are supported */
 };
 
+struct ibv_packet_pacing_caps {
+       uint32_t qp_rate_limit_min; /* Minimum rate limit in kbps */
+       uint32_t qp_rate_limit_max; /* Maximum rate limit in kbps */
+       uint32_t supported_qpts;    /* Bitmap showing which QP types are supported. */
+};
+
 .fi
 .SH "RETURN VALUE"
 .B ibv_query_device_ex()
diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index 29f287b..6606196 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -246,6 +246,12 @@ struct ibv_rss_caps {
 	uint8_t  rx_hash_function; /* enum ibv_rx_hash_function_flags */
 };
 
+struct ibv_packet_pacing_caps {
+	uint32_t qp_rate_limit_min;
+	uint32_t qp_rate_limit_max; /* In kbps */
+	uint32_t supported_qpts;
+};
+
 struct ibv_device_attr_ex {
 	struct ibv_device_attr	orig_attr;
 	uint32_t		comp_mask;
@@ -256,6 +262,7 @@ struct ibv_device_attr_ex {
 	struct ibv_tso_caps	tso_caps;
 	struct ibv_rss_caps     rss_caps;
 	uint32_t		max_wq_type_rq;
+	struct ibv_packet_pacing_caps packet_pacing_caps;
 };
 
 enum ibv_mtu {
-- 
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/3] ibverbs: Add support for packet pacing

[Yishai Hadas]

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

New command ibv_cmd_modify_qp_ex is added to support more QP attributes
such as packet pacing rate limit. Drivers which support packet pacing
should use this command to call the kernel.

Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/cmd.c               | 55 ++++++++++++++++++++++++++++++++++++++----
 libibverbs/driver.h            |  5 ++++
 libibverbs/kern-abi.h          | 27 ++++++++++++++++++---
 libibverbs/libibverbs.map      |  5 ++++
 libibverbs/man/ibv_modify_qp.3 |  2 ++
 libibverbs/verbs.h             |  4 ++-
 6 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/libibverbs/cmd.c b/libibverbs/cmd.c
index a702d67..362c488 100644
--- a/libibverbs/cmd.c
+++ b/libibverbs/cmd.c
@@ -1215,12 +1215,10 @@ int ibv_cmd_query_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 	return 0;
 }
 
-int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
-		      int attr_mask,
-		      struct ibv_modify_qp *cmd, size_t cmd_size)
+static void copy_modify_qp_fields(struct ibv_qp *qp, struct ibv_qp_attr *attr,
+				  int attr_mask,
+				  struct ibv_modify_qp_common *cmd)
 {
-	IBV_INIT_CMD(cmd, cmd_size, MODIFY_QP);
-
 	cmd->qp_handle = qp->handle;
 	cmd->attr_mask = attr_mask;
 
@@ -1344,6 +1342,22 @@ int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 		cmd->dest_qp_num = 0;
 
 	cmd->reserved[0] = cmd->reserved[1] = 0;
+}
+
+int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
+		      int attr_mask,
+		      struct ibv_modify_qp *cmd, size_t cmd_size)
+{
+	/*
+	 * Masks over IBV_QP_DEST_QPN are only supported by
+	 * ibv_cmd_modify_qp_ex.
+	 */
+	if (attr_mask & ~((IBV_QP_DEST_QPN << 1) -1))
+		return EOPNOTSUPP;
+
+	IBV_INIT_CMD(cmd, cmd_size, MODIFY_QP);
+
+	copy_modify_qp_fields(qp, attr, attr_mask, &cmd->base);
 
 	if (write(qp->context->cmd_fd, cmd, cmd_size) != cmd_size)
 		return errno;
@@ -1351,6 +1365,37 @@ int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 	return 0;
 }
 
+int ibv_cmd_modify_qp_ex(struct ibv_qp *qp, struct ibv_qp_attr *attr,
+			 int attr_mask, struct ibv_modify_qp_ex *cmd,
+			 size_t cmd_core_size, size_t cmd_size,
+			 struct ibv_modify_qp_resp_ex *resp,
+			 size_t resp_core_size, size_t resp_size)
+{
+	if (resp_core_size < offsetof(struct ibv_modify_qp_resp_ex,
+			     response_length) + sizeof(resp->response_length))
+		return EINVAL;
+
+	IBV_INIT_CMD_RESP_EX_V(cmd, cmd_core_size, cmd_size, MODIFY_QP_EX,
+			       resp, resp_core_size, resp_size);
+
+	copy_modify_qp_fields(qp, attr, attr_mask, &cmd->base);
+
+	if (attr_mask & IBV_QP_RATE_LIMIT) {
+		if (cmd_size >= offsetof(struct ibv_modify_qp_ex, rate_limit) +
+		    sizeof(cmd->rate_limit))
+			cmd->rate_limit = attr->rate_limit;
+		else
+			return EINVAL;
+	}
+
+	if (write(qp->context->cmd_fd, cmd, cmd_size) != cmd_size)
+		return errno;
+
+	(void)VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
+
+	return 0;
+}
+
 int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 		      struct ibv_send_wr **bad_wr)
 {
diff --git a/libibverbs/driver.h b/libibverbs/driver.h
index ea3dade..53e4409 100644
--- a/libibverbs/driver.h
+++ b/libibverbs/driver.h
@@ -225,6 +225,11 @@ int ibv_cmd_query_qp(struct ibv_qp *qp, struct ibv_qp_attr *qp_attr,
 int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 		      int attr_mask,
 		      struct ibv_modify_qp *cmd, size_t cmd_size);
+int ibv_cmd_modify_qp_ex(struct ibv_qp *qp, struct ibv_qp_attr *attr,
+			 int attr_mask, struct ibv_modify_qp_ex *cmd,
+			 size_t cmd_core_size, size_t cmd_size,
+			 struct ibv_modify_qp_resp_ex *resp,
+			 size_t resp_core_size, size_t resp_size);
 int ibv_cmd_destroy_qp(struct ibv_qp *qp);
 int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 		      struct ibv_send_wr **bad_wr);
diff --git a/libibverbs/kern-abi.h b/libibverbs/kern-abi.h
index bb57495..210dd3e 100644
--- a/libibverbs/kern-abi.h
+++ b/libibverbs/kern-abi.h
@@ -114,6 +114,8 @@ enum {
 					 IB_USER_VERBS_CMD_CREATE_QP,
 	IB_USER_VERBS_CMD_CREATE_CQ_EX = IB_USER_VERBS_CMD_EXTENDED_MASK |
 						IB_USER_VERBS_CMD_CREATE_CQ,
+	IB_USER_VERBS_CMD_MODIFY_QP_EX = IB_USER_VERBS_CMD_EXTENDED_MASK |
+						IB_USER_VERBS_CMD_MODIFY_QP,
 	IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_EXTENDED_MASK +
 					IB_USER_VERBS_CMD_THRESHOLD,
 	IB_USER_VERBS_CMD_DESTROY_FLOW,
@@ -745,10 +747,7 @@ struct ibv_query_qp_resp {
 	__u64 driver_data[0];
 };
 
-struct ibv_modify_qp {
-	__u32 command;
-	__u16 in_words;
-	__u16 out_words;
+struct ibv_modify_qp_common {
 	struct ibv_qp_dest dest;
 	struct ibv_qp_dest alt_dest;
 	__u32 qp_handle;
@@ -775,9 +774,28 @@ struct ibv_modify_qp {
 	__u8  alt_port_num;
 	__u8  alt_timeout;
 	__u8  reserved[2];
+};
+
+struct ibv_modify_qp {
+	__u32 command;
+	__u16 in_words;
+	__u16 out_words;
+	struct ibv_modify_qp_common base;
 	__u64 driver_data[0];
 };
 
+struct ibv_modify_qp_ex {
+	struct ex_hdr		    hdr;
+	struct ibv_modify_qp_common base;
+	__u32  rate_limit;
+	__u32  reserved;
+};
+
+struct ibv_modify_qp_resp_ex {
+	__u32  comp_mask;
+	__u32  response_length;
+};
+
 struct ibv_destroy_qp {
 	__u32 command;
 	__u16 in_words;
@@ -1187,6 +1205,7 @@ enum {
 	IB_USER_VERBS_CMD_DESTROY_WQ_V2 = -1,
 	IB_USER_VERBS_CMD_CREATE_RWQ_IND_TBL_V2 = -1,
 	IB_USER_VERBS_CMD_DESTROY_RWQ_IND_TBL_V2 = -1,
+	IB_USER_VERBS_CMD_MODIFY_QP_EX_V2 = -1,
 };
 
 struct ibv_modify_srq_v3 {
diff --git a/libibverbs/libibverbs.map b/libibverbs/libibverbs.map
index 33cd6b6..f697b69 100644
--- a/libibverbs/libibverbs.map
+++ b/libibverbs/libibverbs.map
@@ -131,3 +131,8 @@ IBVERBS_1.3 {
 		ibv_cmd_destroy_rwq_ind_table;
 		ibv_query_gid_type;
 } IBVERBS_1.1;
+
+IBVERBS_1.4 {
+	global:
+		ibv_cmd_modify_qp_ex;
+} IBVERBS_1.3;
diff --git a/libibverbs/man/ibv_modify_qp.3 b/libibverbs/man/ibv_modify_qp.3
index 66a4974..487aa94 100644
--- a/libibverbs/man/ibv_modify_qp.3
+++ b/libibverbs/man/ibv_modify_qp.3
@@ -49,6 +49,7 @@ uint8_t                 retry_cnt;              /* Retry count (valid only for R
 uint8_t                 rnr_retry;              /* RNR retry (valid only for RC QPs) */
 uint8_t                 alt_port_num;           /* Alternate port number */
 uint8_t                 alt_timeout;            /* Local ack timeout for alternate path (valid only for RC QPs) */
+uint32_t                rate_limit;             /* Rate limit in kbps for packet pacing */
 .in -8
 };
 .fi
@@ -105,6 +106,7 @@ The argument is either 0 or the bitwise OR of one or more of the following flags
 .B IBV_QP_CAP \fR Set cap
 .TP
 .B IBV_QP_DEST_QPN \fR Set dest_qp_num
+.B IBV_QP_RATE_LIMIT \fR Set rate_limit
 .SH "RETURN VALUE"
 .B ibv_modify_qp()
 returns 0 on success, or the value of errno on failure (which indicates the failure reason).
diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index 6606196..27726b5 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -829,7 +829,8 @@ enum ibv_qp_attr_mask {
 	IBV_QP_MAX_DEST_RD_ATOMIC	= 1 << 17,
 	IBV_QP_PATH_MIG_STATE		= 1 << 18,
 	IBV_QP_CAP			= 1 << 19,
-	IBV_QP_DEST_QPN			= 1 << 20
+	IBV_QP_DEST_QPN			= 1 << 20,
+	IBV_QP_RATE_LIMIT		= 1 << 25,
 };
 
 enum ibv_qp_state {
@@ -875,6 +876,7 @@ struct ibv_qp_attr {
 	uint8_t			rnr_retry;
 	uint8_t			alt_port_num;
 	uint8_t			alt_timeout;
+	uint32_t		rate_limit;
 };
 
 enum ibv_wr_opcode {
-- 
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/3] mlx5: Add packet pacing support

[Yishai Hadas]

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

Add packet pacing support, it includes:
- Using the extended command to pass rate limit
  to the kernel. (i.e. ibv_cmd_modify_qp_ex).
- Reporting device capabilities.

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 |  2 ++
 providers/mlx5/verbs.c    | 20 ++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index 4b5b577..38220ec 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -253,6 +253,8 @@ 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 ibv_packet_pacing_caps	packet_pacing_caps;
 };
 
 #endif /* MLX5_ABI_H */
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index e288ebf..b4bc228 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1498,10 +1498,16 @@ int mlx5_query_qp(struct ibv_qp *ibqp, struct ibv_qp_attr *attr,
 	return 0;
 }
 
+enum {
+	MLX5_MODIFY_QP_EX_ATTR_MASK = IBV_QP_RATE_LIMIT,
+};
+
 int mlx5_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 		   int attr_mask)
 {
-	struct ibv_modify_qp cmd;
+	struct ibv_modify_qp_ex cmd_ex = {};
+	struct ibv_modify_qp_resp_ex resp = {};
+	struct ibv_modify_qp cmd = {};
 	struct mlx5_qp *mqp = to_mqp(qp);
 	struct mlx5_context *context = to_mctx(qp->context);
 	int ret;
@@ -1533,7 +1539,15 @@ int mlx5_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
 		}
 	}
 
-	ret = ibv_cmd_modify_qp(qp, attr, attr_mask, &cmd, sizeof(cmd));
+	if (attr_mask & MLX5_MODIFY_QP_EX_ATTR_MASK)
+		ret = ibv_cmd_modify_qp_ex(qp, attr, attr_mask,
+					   &cmd_ex,
+					   sizeof(cmd_ex), sizeof(cmd_ex),
+					   &resp,
+					   sizeof(resp), sizeof(resp));
+	else
+		ret = ibv_cmd_modify_qp(qp, attr, attr_mask,
+					&cmd, sizeof(cmd));
 
 	if (!ret		       &&
 	    (attr_mask & IBV_QP_STATE) &&
@@ -1887,6 +1901,8 @@ 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;
+
 	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

Re: [PATCH rdma-core 2/3] ibverbs: Add support for packet pacing

[Jason Gunthorpe]

On Sun, Jan 08, 2017 at 06:32:27PM +0200, Yishai Hadas wrote:
> +int ibv_cmd_modify_qp_ex(struct ibv_qp *qp, struct ibv_qp_attr *attr,
> +			 int attr_mask, struct ibv_modify_qp_ex *cmd,
> +			 size_t cmd_core_size, size_t cmd_size,
> +			 struct ibv_modify_qp_resp_ex *resp,
> +			 size_t resp_core_size, size_t resp_size)
> +{
> +	if (resp_core_size < offsetof(struct ibv_modify_qp_resp_ex,
> +			     response_length) + sizeof(resp->response_length))
> +		return EINVAL;
> +
> +	IBV_INIT_CMD_RESP_EX_V(cmd, cmd_core_size, cmd_size, MODIFY_QP_EX,
> +			       resp, resp_core_size, resp_size);
> +
> +	copy_modify_qp_fields(qp, attr, attr_mask, &cmd->base);
> +
> +	if (attr_mask & IBV_QP_RATE_LIMIT) {
> +		if (cmd_size >= offsetof(struct ibv_modify_qp_ex, rate_limit) +
> +		    sizeof(cmd->rate_limit))
> +			cmd->rate_limit = attr->rate_limit;
> +		else
> +			return EINVAL;
> +	}

cmd->rate_limit is not being 0'd if it isn't set

> @@ -829,7 +829,8 @@ enum ibv_qp_attr_mask {
>  	IBV_QP_MAX_DEST_RD_ATOMIC	= 1 << 17,
>  	IBV_QP_PATH_MIG_STATE		= 1 << 18,
>  	IBV_QP_CAP			= 1 << 19,
> -	IBV_QP_DEST_QPN			= 1 << 20
> +	IBV_QP_DEST_QPN			= 1 << 20,
> +	IBV_QP_RATE_LIMIT		= 1 << 25,
>  };

Why 25? 21 I think.

>  enum ibv_qp_state {
> @@ -875,6 +876,7 @@ struct ibv_qp_attr {
>  	uint8_t			rnr_retry;
>  	uint8_t			alt_port_num;
>  	uint8_t			alt_timeout;
> +	uint32_t		rate_limit;
>  };

Nope, this is returned by query_qp so you cannot just extend it.

You are supporting query_qp, right?

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 3/3] mlx5: Add packet pacing support

[Jason Gunthorpe]

On Sun, Jan 08, 2017 at 06:32:28PM +0200, Yishai Hadas wrote:
> +	if (attr_mask & MLX5_MODIFY_QP_EX_ATTR_MASK)
> +		ret = ibv_cmd_modify_qp_ex(qp, attr, attr_mask,
> +					   &cmd_ex,
> +					   sizeof(cmd_ex), sizeof(cmd_ex),
> +					   &resp,
> +					   sizeof(resp), sizeof(resp));
> +	else
> +		ret = ibv_cmd_modify_qp(qp, attr, attr_mask,
> +					&cmd, sizeof(cmd));
>

What is this if doing? Is it trying to retain compat with old kernels?

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 2/3] ibverbs: Add support for packet pacing

[Yishai Hadas]

On 1/9/2017 5:38 PM, Jason Gunthorpe wrote:
> On Sun, Jan 08, 2017 at 06:32:27PM +0200, Yishai Hadas wrote:
>> +int ibv_cmd_modify_qp_ex(struct ibv_qp *qp, struct ibv_qp_attr *attr,
>> +			 int attr_mask, struct ibv_modify_qp_ex *cmd,
>> +			 size_t cmd_core_size, size_t cmd_size,
>> +			 struct ibv_modify_qp_resp_ex *resp,
>> +			 size_t resp_core_size, size_t resp_size)
>> +{
>> +	if (resp_core_size < offsetof(struct ibv_modify_qp_resp_ex,
>> +			     response_length) + sizeof(resp->response_length))
>> +		return EINVAL;
>> +
>> +	IBV_INIT_CMD_RESP_EX_V(cmd, cmd_core_size, cmd_size, MODIFY_QP_EX,
>> +			       resp, resp_core_size, resp_size);
>> +
>> +	copy_modify_qp_fields(qp, attr, attr_mask, &cmd->base);
>> +
>> +	if (attr_mask & IBV_QP_RATE_LIMIT) {
>> +		if (cmd_size >= offsetof(struct ibv_modify_qp_ex, rate_limit) +
>> +		    sizeof(cmd->rate_limit))
>> +			cmd->rate_limit = attr->rate_limit;
>> +		else
>> +			return EINVAL;
>> +	}
>
> cmd->rate_limit is not being 0'd if it isn't set

The 'cmd' was fully set to 0'd by the provider, see usage in the last patch.

In case we want to do it in this layer I would vote on using memset at 
the beginning of that function to prevent repeating this for any new 
field. Your last patch in that area could do it as well.

>> @@ -829,7 +829,8 @@ enum ibv_qp_attr_mask {
>>  	IBV_QP_MAX_DEST_RD_ATOMIC	= 1 << 17,
>>  	IBV_QP_PATH_MIG_STATE		= 1 << 18,
>>  	IBV_QP_CAP			= 1 << 19,
>> -	IBV_QP_DEST_QPN			= 1 << 20
>> +	IBV_QP_DEST_QPN			= 1 << 20,
>> +	IBV_QP_RATE_LIMIT		= 1 << 25,
>>  };
>
> Why 25? 21 I think.

To match kernel, 21-24 are reserved.

>
>>  enum ibv_qp_state {
>> @@ -875,6 +876,7 @@ struct ibv_qp_attr {
>>  	uint8_t			rnr_retry;
>>  	uint8_t			alt_port_num;
>>  	uint8_t			alt_timeout;
>> +	uint32_t		rate_limit;
>>  };
>
> Nope, this is returned by query_qp so you cannot just extend it.
It's safe, please see below.

>
> You are supporting query_qp, right?
At the moment the kernel doesn't support an extended command for 
query_qp so user space doesn't get and touch this field.

Once support will be added in the kernel, the attr_mask field of 
ibv_query_qp should be used to indicate whether application asked for 
that field (i.e. IBV_QP_RATE_LIMIT) and supplied an input structure that 
has memory to hold this output so it's safe to extend the above 
structure. Same idea was used in this series for accessing this field in 
modify_qp as an input.


We can consider adding an extra check in ibv_cmd_query_qp as part of 
this series to check whether IBV_QP_RATE_LIMIT or higher bits were 
asked, if yes return an error as there is no support for that value 
without using an extended command, makes sense ?

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

--
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 3/3] mlx5: Add packet pacing support

[Yishai Hadas]

On 1/9/2017 5:39 PM, Jason Gunthorpe wrote:
> On Sun, Jan 08, 2017 at 06:32:28PM +0200, Yishai Hadas wrote:
>> +	if (attr_mask & MLX5_MODIFY_QP_EX_ATTR_MASK)
>> +		ret = ibv_cmd_modify_qp_ex(qp, attr, attr_mask,
>> +					   &cmd_ex,
>> +					   sizeof(cmd_ex), sizeof(cmd_ex),
>> +					   &resp,
>> +					   sizeof(resp), sizeof(resp));
>> +	else
>> +		ret = ibv_cmd_modify_qp(qp, attr, attr_mask,
>> +					&cmd, sizeof(cmd));
>>
>
> What is this if doing? Is it trying to retain compat with old kernels?

Correct, only when the input mask requires the extended command it will 
be used, calling the extended command always will fail in legacy kernels 
as they don't support it, need to prevent 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 rdma-core 2/3] ibverbs: Add support for packet pacing

[Jason Gunthorpe]

On Mon, Jan 09, 2017 at 06:46:33PM +0200, Yishai Hadas wrote:

> >cmd->rate_limit is not being 0'd if it isn't set
> 
> The 'cmd' was fully set to 0'd by the provider, see usage in the last patch.

Why are some providers zeroing this and other are not?

> In case we want to do it in this layer I would vote on using memset at the
> beginning of that function to prevent repeating this for any new field. Your
> last patch in that area could do it as well.

Sounds like you should clean this all up and use = {} consistently in
the providers and drop the explicit = 0.

> >>@@ -829,7 +829,8 @@ enum ibv_qp_attr_mask {
> >> 	IBV_QP_MAX_DEST_RD_ATOMIC	= 1 << 17,
> >> 	IBV_QP_PATH_MIG_STATE		= 1 << 18,
> >> 	IBV_QP_CAP			= 1 << 19,
> >>-	IBV_QP_DEST_QPN			= 1 << 20
> >>+	IBV_QP_DEST_QPN			= 1 << 20,
> >>+	IBV_QP_RATE_LIMIT		= 1 << 25,
> >> };
> >
> >Why 25? 21 I think.
> 
> To match kernel, 21-24 are reserved.

Hm, Ok

> >
> >> enum ibv_qp_state {
> >>@@ -875,6 +876,7 @@ struct ibv_qp_attr {
> >> 	uint8_t			rnr_retry;
> >> 	uint8_t			alt_port_num;
> >> 	uint8_t			alt_timeout;
> >>+	uint32_t		rate_limit;
> >> };
> >
> >Nope, this is returned by query_qp so you cannot just extend it.
> It's safe, please see below.
 
> >
> >You are supporting query_qp, right?
> At the moment the kernel doesn't support an extended command for query_qp so
> user space doesn't get and touch this field.

Yuk, why would you do this?

> Once support will be added in the kernel, the attr_mask field of
> ibv_query_qp should be used to indicate whether application asked for that
> field (i.e. IBV_QP_RATE_LIMIT) and supplied an input structure that has
> memory to hold this output so it's safe to extend the above structure. Same
> idea was used in this series for accessing this field in modify_qp as an
> input.

Insane as it may be, 'attr_mask' is defined as a hint for
ibv_query_qp, you are redefining it to be a hard requirement for ABI
purposes.

> We can consider adding an extra check in ibv_cmd_query_qp as part of this
> series to check whether IBV_QP_RATE_LIMIT or higher bits were asked, if yes
> return an error as there is no support for that value without using an
> extended command, makes sense ?

Absolutely.

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 2/3] ibverbs: Add support for packet pacing

[Yishai Hadas]

On 1/9/2017 7:02 PM, Jason Gunthorpe wrote:
> On Mon, Jan 09, 2017 at 06:46:33PM +0200, Yishai Hadas wrote:
>
>>> cmd->rate_limit is not being 0'd if it isn't set
>>
>> The 'cmd' was fully set to 0'd by the provider, see usage in the last patch.
>
> Why are some providers zeroing this and other are not?

ibv_cmd_modify_qp_ex is used only by mlx5 and it uses the = {} 
initialization.
>
>> In case we want to do it in this layer I would vote on using memset at the
>> beginning of that function to prevent repeating this for any new field. Your
>> last patch in that area could do it as well.
>
> Sounds like you should clean this all up and use = {} consistently in
> the providers and drop the explicit = 0.

We can add some pre-patch to the series to make the 
ibv_cmd_modify/ibv_cmd_modify_ex behaves similarly by cleaning up the 
explicit = 0 and use = {} in all the providers.

>> We can consider adding an extra check in ibv_cmd_query_qp as part of this
>> series to check whether IBV_QP_RATE_LIMIT or higher bits were asked, if yes
>> return an error as there is no support for that value without using an
>> extended command, makes sense ?
>
> Absolutely.

Will add this check as part of V1 and return an error for that attr_mask 
value (i.e IBV_QP_RATE_LIMIT) and for higher future bits.

When the extended query_qp command in the kernel will be added we can 
re-discuss whether attr_mask which comes from the application as part of 
ibv_query_qp can serve as an output indication and we can stay with 
current API from application point of view. My personal tendency is go 
with that approach and let applications use one API and handle the 
choice whether to call the legacy or the extended command internally 
based on attr_mask.



--
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 2/3] ibverbs: Add support for packet pacing

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Mon, Jan 09, 2017 at 11:44:55PM +0200, Yishai Hadas wrote:
> On 1/9/2017 7:02 PM, Jason Gunthorpe wrote:
> > On Mon, Jan 09, 2017 at 06:46:33PM +0200, Yishai Hadas wrote:
> >
> > > In case we want to do it in this layer I would vote on using memset at the
> > > beginning of that function to prevent repeating this for any new field. Your
> > > last patch in that area could do it as well.
> >
> > Sounds like you should clean this all up and use = {} consistently in
> > the providers and drop the explicit = 0.
>
> We can add some pre-patch to the series to make the
> ibv_cmd_modify/ibv_cmd_modify_ex behaves similarly by cleaning up the
> explicit = 0 and use = {} in all the providers.

Yes, please do it and send that patch as standalone cleanup before
series, so we will be able to apply it without waiting till packet
pacing is accepted.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]