Expose CQ moderation to user space

Changelog v0-v1:
* Dropped comp_mask, there is already attr_mask
* Put CQ moderartion into special struct instead of general struct ib_uverbs_ex_modify_cq

---------------------------------------
This patch set exposes CQ moderation. This will allow to moderate number of CQEs needed to create an event. Such change brings performance improvement by reducing pressure on application to receive event per-CQE.

The proposed semantics follows the well-established kernel semantics.

* cq_max_count - defines the number of cookies needed to create an event.
* cq_period - defines the timeout between last event and a new one
that will occur even if cq_max_count was not reached

The patches are available in the git repository at:
git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git tags/rdma-next-2017-10-29-1

Thanks
---------------------------------------

Yonatan Cohen (6):
IB/uverbs: Allow CQ moderation with modify CQ IB/mlx4: Exposing modify CQ callback to uverbs layer IB/mlx5: Exposing modify CQ callback to uverbs layer IB/uverbs: Add CQ moderation capability to query_device IB/mlx4: Add CQ moderation capability to query_device IB/mlx5: Add CQ moderation capability to query_device

 drivers/infiniband/core/uverbs.h      |  1 +
 drivers/infiniband/core/uverbs_cmd.c  | 51 +++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/uverbs_main.c |  1 +
 drivers/infiniband/hw/mlx4/main.c     |  6 +++++
 drivers/infiniband/hw/mlx4/mlx4_ib.h  |  1 +
 drivers/infiniband/hw/mlx5/cq.c       |  3 +++
 drivers/infiniband/hw/mlx5/main.c     | 10 ++++++-
 include/linux/mlx4/cq.h               |  3 +++
 include/linux/mlx5/cq.h               |  3 +++
 include/rdma/ib_verbs.h               | 10 +++++++
 include/uapi/rdma/ib_user_verbs.h     | 22 ++++++++++++++-
 11 files changed, 109 insertions(+), 2 deletions(-)

--
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

Uverbs support in modify_cq for CQ moderation only. Gives ability to change cq_max_count and cq_period.
CQ moderation enhance performance by moderating the number of cookies needed to create an event instead of application having to suffer from event per cookie.
To achieve CQ moderation the application needs to set cq_max_count and cq_period.
cq_max_count - defines the number of cookies
needed to create an event.
cq_period - defines the timeout (micro seconds) between last
event and a new one that will occur even if cq_max_count was not satisfied

---
 drivers/infiniband/core/uverbs.h      |  1 +
 drivers/infiniband/core/uverbs_cmd.c  | 42 +++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/uverbs_main.c |  1 +
 include/rdma/ib_verbs.h               |  4 ++++
 include/uapi/rdma/ib_user_verbs.h     | 15 ++++++++++++-
 5 files changed, 62 insertions(+), 1 deletion(-)

index ee2739ae4305..deccefb71a6b 100644
@@ -306,5 +306,6 @@ IB_UVERBS_DECLARE_EX_CMD(destroy_wq);
 IB_UVERBS_DECLARE_EX_CMD(create_rwq_ind_table);
 IB_UVERBS_DECLARE_EX_CMD(destroy_rwq_ind_table);
 IB_UVERBS_DECLARE_EX_CMD(modify_qp);
+IB_UVERBS_DECLARE_EX_CMD(modify_cq);
 
 #endif /* UVERBS_H */

index b5febfd84ee5..381fd9c096ae 100644
@@ -128,6 +128,7 @@ static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
 	[IB_USER_VERBS_EX_CMD_CREATE_RWQ_IND_TBL] = ib_uverbs_ex_create_rwq_ind_table,
 	[IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL] = ib_uverbs_ex_destroy_rwq_ind_table,
 	[IB_USER_VERBS_EX_CMD_MODIFY_QP]        = ib_uverbs_ex_modify_qp,
+	[IB_USER_VERBS_EX_CMD_MODIFY_CQ]        = ib_uverbs_ex_modify_cq,
 };
 
 static void ib_uverbs_add_one(struct ib_device *device);

index 0b671982bbb3..8e0d3780ce4e 100644
@@ -311,6 +311,10 @@ struct ib_cq_init_attr {
 	u32		flags;
 };
 
+enum ib_cq_attr_mask {
+	IB_CQ_MODERATE = 1 << 0,
+};
+
 struct ib_device_attr {
 	u64			fw_ver;
 	__be64			sys_image_guid;

index d4e0b53bfc75..cfa09d6095d6 100644
@@ -100,7 +100,8 @@ enum {
 	IB_USER_VERBS_EX_CMD_MODIFY_WQ,
 	IB_USER_VERBS_EX_CMD_DESTROY_WQ,
 	IB_USER_VERBS_EX_CMD_CREATE_RWQ_IND_TBL,
-	IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL
+	IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
+	IB_USER_VERBS_EX_CMD_MODIFY_CQ
 };
 
 /*

@@ -1150,6 +1151,18 @@ struct ib_uverbs_ex_destroy_rwq_ind_table  {
 	__u32 ind_tbl_handle;
 };
 
+struct ib_uverbs_cq_moderation {
+	__u16 cq_count;
+	__u16 cq_period;
+};
+
+struct ib_uverbs_ex_modify_cq {
+	__u32 cq_handle;
+	__u32 attr_mask;
+	struct ib_uverbs_cq_moderation attr;
+	__u32 reserved;
+};
+
 #define IB_DEVICE_NAME_MAX 64
 
 #endif /* IB_USER_VERBS_H */

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Like the other recent uAPI patches, lets us see the rdma-core side and man page first please..

Can you organize all of them into a rdma-core branch someplace?

Not really sure what a cookie is, or why this should be needed when we already have the means to indicate if CQE's should be signalled or not.

I guess it is for UD applications where the existing signalled stuff isn't applicable?

Yishai planned to send it tomorrow.

It is my fault, I forgot to change cookie to be CQE.

It follows already existed ib_modify_cq(), see commit 2dd571622787 ("IB/core: Add support for modify CQ")

And that function should have been called set_cq_moderation:

+ * ib_modify_cq - Modifies moderation params of the CQ
+ * @cq: The CQ to modify.
+ * @cq_count: number of CQEs that will trigger an event
+ * @cq_period: max period of time in usec before triggering an event
+ *
+ */
+int ib_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period);

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I see it differently, this is extendable version of modify_cq, which is going to benefit all other users who will decide to extend it.

Thanks

The only reason we have these goofy 'thick' APIs like modify_qp is because people said they needed a very high rate of qp modifications and could not tolerate the performance downside of a dis-aggregated API.

I don't think CQ falls into that reasoning, so it should be dis-aggregated.

Many things in verbs are terrible examples of how to make APIs, we should not just blindly copy things..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

"Many things" ???
At least, you found something that is not terrible, I didn't find yet.

For the libibverb's API, I completely agree with you that we need to strive to more simple API without overloaded functions with a lot of parameters and you can count on my voice and support for any effort to do so.

However, I don't think that kernel to libibverbs API should follow the same path. The centralized entry points to the kernel provides better enforcement and minimizes system call bloat.

Thanks

How so? I didn't think selinux intersected with the CQs at all.. I've never worried about the # of verbs entries, we have so many already.

I also don't like the modify semantic, because IMHO, modify should mirror create, and we don't specify the moderation parameters during create.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Not SELinux enforcement, but various copy_{to|from}_user checks, unified return values, easy folding in case of errors, e.t.c.

It is a matter of personal view and less technical thing. You prefer bazillion entry points to the kernel and I think that such design good for user-space only, and mostly not applicable for kernel<->user interfaces.

It is completely different thing, no one needed it, so no one added it.

Even for the user area having a dedicated 'setter' per field might bring some redundant overhead and limitation. This will prevent the option from application to modify few fields in one API and resulted in a system call per field. Also libibverbs and user drivers may need to maintain some wrapping over the kernel command which in turn might lead to some extra/duplication of code. I would vote to follow modify QP/SRQ/WQ by having one API with an 'attr_mask' field as this series has introduced in both kernel and user.

The matching user space series was published and can be seen at:
https://github.com/linux-rdma/rdma-core/pull/238
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

As a general rule, more simpler apis are saner than single complex apis. It is much easier to understand and document, the error codes work more sanely for debugging and generally needs less 'discovery' support.

And this is always the argument someone brings up - and as I said, I don't see how it really applies to the CQ where we don't expect to create them at a high rate.

Yes, I'm not super excited about having them be different..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The above argument could be issued also on SRQ/WQ which are not expected to be changed in a high rate but uses the attr_mask notation with one system call.

As this is currently the spirit of modify_xxx verbs in both user and kernel, we may stay with that and consider this change for verbs2.0 all around when it will come. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yep, those perhaps should not have used the attr_mask scheme either

Well, there seem to be quite a lot of new API proposals now, and stuffing each of them through some goofy comp_mask or attr_mask thing seems like it is going to create a huge complex mess..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

But they have it as this is current spirit in verbs.

I don't agree to above statement, attr_mask is well defined and easy to be used in modify_xxx verbs. It can clearly mark which attributes are going to be modified similar to some flags attribute. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

If it's the extendable version, then it should have just passed the attr struct (or equivalent), it shouldn't have spelled out the moderation parameters in the function signature. So, either we need to change the signature of ib_modify_cq to a generic, extendable signature, or we need to change the name as Jason points out so we match name and parameter signature in the same spirit.

Also, as you point out, need to update the log message to not use cookie.

Let's please make this consistent before merging. BTW, because so much of the rest of the API uses things like modify_qp with an attr struct and a single entry point, I'm leaning towards following that here for the sake of API consistency. Although I can see Jason's point about having simpler entry points for the API, I don't think it buys us anything in the RDMA space because we already have to be aware of and use the monolithic single entry point API style. I'm more inclined to think a single API style is better than mixed API styles, just like mixed coding styles is bad.

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

The kernel side should focus on setting cq moderation, as I understand we have ULPs that want to adjust this dynamically so higher performance through a simplified API makes the most sense. No need to mirror libibverbs.

Should we introduce a create_cq_ex as well at this point? It is really weird in verbs to have a modify attrs without a create also accepting the attrs.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I'll fix commit log, rebase and resubmit, but it is not clear to me the benefits of changing kernel version of modify_cq and all their users to extended version. I think that we better convert them once they actually will require it.

Thanks

We don't need the kernel to follow user space, modify_cq should just be renamed to set_cq_moderation to avoid confusion and be done with it.

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

Exposed mlx4_ib_modify_cq to be called from ib device verb list.

---
 drivers/infiniband/hw/mlx4/main.c    | 3 +++
 drivers/infiniband/hw/mlx4/mlx4_ib.h | 1 +
 include/linux/mlx4/cq.h              | 3 +++
 3 files changed, 7 insertions(+)

index c636842c5be0..58845e67dad1 100644
@@ -2733,6 +2733,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 	ibdev->ib_dev.get_dev_fw_str    = get_fw_ver_str;
 	ibdev->ib_dev.disassociate_ucontext = mlx4_ib_disassociate_ucontext;
 
+	ibdev->ib_dev.uverbs_ex_cmd_mask |=
+		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_CQ);
+
 	if ((dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS) &&
 	    ((mlx4_ib_port_link_layer(&ibdev->ib_dev, 1) ==
 	    IB_LINK_LAYER_ETHERNET) ||

index 1fa19820355a..9a55064ed69a 100644
@@ -47,6 +47,7 @@
 #include <linux/mlx4/device.h>
 #include <linux/mlx4/doorbell.h>
 #include <linux/mlx4/qp.h>
+#include <linux/mlx4/cq.h>
 
 #define MLX4_IB_DRV_NAME	"mlx4_ib"
 

index 09cebe528488..508e8cc5ee86 100644
@@ -136,6 +136,9 @@ enum {
 	MLX4_CQE_BAD_FCS                 = 1 << 4,
 };
 
+#define MLX4_MAX_CQ_PERIOD (BIT(16) - 1)
+#define MLX4_MAX_CQ_COUNT (BIT(16) - 1)
+
 static inline void mlx4_cq_arm(struct mlx4_cq *cq, u32 cmd,
 			       void __iomem *uar_page,
 			       spinlock_t *doorbell_lock)

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

Exposed mlx5_ib_modify_cq to be called from ib device verb list.

---
 drivers/infiniband/hw/mlx5/cq.c   | 3 +++
 drivers/infiniband/hw/mlx5/main.c | 3 ++-
 include/linux/mlx5/cq.h           | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

index 01b218a3c277..18705cbcdc8c 100644
@@ -1149,6 +1149,9 @@ int mlx5_ib_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period)
 	if (!MLX5_CAP_GEN(dev->mdev, cq_moderation))
 		return -ENOSYS;
 
+	if (cq_period > MLX5_MAX_CQ_PERIOD)
+		return -EINVAL;
+
 	err = mlx5_core_modify_cq_moderation(dev->mdev, &mcq->mcq,
 					     cq_period, cq_count);
 	if (err)

index e5f8d5d1cd7e..838c47012388 100644
@@ -4020,7 +4020,8 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
 		(1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE)	|
 		(1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ)	|
 		(1ull << IB_USER_VERBS_EX_CMD_CREATE_QP)	|
-		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_QP);
+		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_QP)	|
+		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_CQ);
 
 	dev->ib_dev.query_device	= mlx5_ib_query_device;
 	dev->ib_dev.query_port		= mlx5_ib_query_port;

index cc718e245b1e..6be357b219ec 100644
@@ -128,6 +128,9 @@ enum {
 	CQE_SIZE_128_PAD = 2,
 };
 
+#define MLX5_MAX_CQ_PERIOD (BIT(__mlx5_bit_sz(cqc, cq_period)) - 1)
+#define MLX5_MAX_CQ_COUNT (BIT(__mlx5_bit_sz(cqc, cq_max_count)) - 1)
+
 static inline int cqe_sz_to_mlx_sz(u8 size, int padding_128_en)
 {
 	return padding_128_en ? CQE_SIZE_128_PAD :

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

The query_device function can now obtain the maximum values for cq_max_count and cq_period, needed for CQ moderation. cq_max_count is a 16 bits number that determines the number of cookies to accumulate before generating an event. cq_period is a 16 bits number that detemines the timeout in micro seconds from the last event generated, upon which a new event will be generated even if cq_max_count was not reached.

---
 drivers/infiniband/core/uverbs_cmd.c | 9 +++++++++
 include/rdma/ib_verbs.h              | 6 ++++++
 include/uapi/rdma/ib_user_verbs.h    | 7 +++++++
 3 files changed, 22 insertions(+)

index 3c2673cd4090..53143e4b1c50 100644
@@ -3852,6 +3852,15 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
 	resp.tm_caps.max_sge		= attr.tm_caps.max_sge;
 	resp.tm_caps.flags		= attr.tm_caps.flags;
 	resp.response_length += sizeof(resp.tm_caps);
+
+	if (ucore->outlen < resp.response_length + sizeof(resp.cq_moderation_caps))
+		goto end;
+
+	resp.cq_moderation_caps.max_cq_moderation_count  =
+		attr.cq_caps.max_cq_moderation_count;
+	resp.cq_moderation_caps.max_cq_moderation_period =
+		attr.cq_caps.max_cq_moderation_period;
+	resp.response_length += sizeof(resp.cq_moderation_caps);
 end:
 	err = ib_copy_to_udata(ucore, &resp, resp.response_length);
 	return err;

index 8e0d3780ce4e..0b484c023fa9 100644
@@ -315,6 +315,11 @@ enum ib_cq_attr_mask {
 	IB_CQ_MODERATE = 1 << 0,
 };
 
+struct ib_cq_caps {
+	u16     max_cq_moderation_count;
+	u16     max_cq_moderation_period;
+};
+
 struct ib_device_attr {
 	u64			fw_ver;
 	__be64			sys_image_guid;

@@ -365,6 +370,7 @@ struct ib_device_attr {
 	u32			max_wq_type_rq;
 	u32			raw_packet_caps; /* Use ib_raw_packet_caps enum */
 	struct ib_tm_caps	tm_caps;
+	struct ib_cq_caps       cq_caps;
 };
 
 enum ib_mtu {

index cfa09d6095d6..5186fb12629b 100644
@@ -125,6 +125,12 @@ struct ib_uverbs_comp_event_desc {
 	__u64 cq_handle;
 };
 
+struct ib_uverbs_cq_moderation_caps {
+	__u16     max_cq_moderation_count;
+	__u16     max_cq_moderation_period;
+	__u32     reserved;
+};
+
 /*
  * All commands from userspace should start with a __u32 command field
  * followed by __u16 in_words and out_words fields (which give the

@@ -263,6 +269,7 @@ struct ib_uverbs_ex_query_device_resp {
 	__u32  max_wq_type_rq;
 	__u32 raw_packet_caps;
 	struct ib_uverbs_tm_caps tm_caps;
+	struct ib_uverbs_cq_moderation_caps cq_moderation_caps;
 };
 
 struct ib_uverbs_query_port {

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

query_device can now obtain the maximum values for cq_max_count and cq_period, needed for cq moderation.

---
 drivers/infiniband/hw/mlx4/main.c | 3 +++
 1 file changed, 3 insertions(+)

index 58845e67dad1..c6420e26f2d8 100644
@@ -563,6 +563,9 @@ static int mlx4_ib_query_device(struct ib_device *ibdev,
 		props->max_wq_type_rq = props->max_qp;
 	}
 
+	props->cq_caps.max_cq_moderation_count = MLX4_MAX_CQ_COUNT;
+	props->cq_caps.max_cq_moderation_period = MLX4_MAX_CQ_PERIOD;
+
 	if (!mlx4_is_slave(dev->dev))
 		err = mlx4_get_internal_clock_params(dev->dev, &clock_params);
 

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/[email protected]>

query_device can now obtain the maximum values for cq_max_count and cq_period, needed for cq moderation.

---
 drivers/infiniband/hw/mlx5/main.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 838c47012388..18fe01826942 100644
@@ -790,6 +790,13 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,
 		props->tm_caps.max_sge = MLX5_TM_MAX_SGE;
 	}
 
+	if (MLX5_CAP_GEN(dev->mdev, cq_moderation)) {
+		props->cq_caps.max_cq_moderation_count =
+						MLX5_MAX_CQ_COUNT;
+		props->cq_caps.max_cq_moderation_period =
+						MLX5_MAX_CQ_PERIOD;
+	}
+
 	if (field_avail(typeof(resp), cqe_comp_caps, uhw->outlen)) {
 		resp.cqe_comp_caps.max_num =
 			MLX5_CAP_GEN(dev->mdev, cqe_compression) ?

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html