[PATCH rdma-next v1 0/6] Expose CQ moderation to user space

[PATCH rdma-next v1 0/6] Expose CQ moderation to user space

[Leon Romanovsky]

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index ee2739ae4305..deccefb71a6b 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -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 */
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 8ca36843ef38..3c2673cd4090 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3856,3 +3856,45 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
 	err = ib_copy_to_udata(ucore, &resp, resp.response_length);
 	return err;
 }
+
+int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
+			   struct ib_device *ib_dev,
+			   struct ib_udata *ucore,
+			   struct ib_udata *uhw)
+{
+	struct ib_uverbs_ex_modify_cq cmd = {};
+	struct ib_cq *cq;
+	size_t required_cmd_sz;
+	int ret;
+
+	required_cmd_sz = offsetof(typeof(cmd), reserved) +
+				sizeof(cmd.reserved);
+	if (ucore->inlen < required_cmd_sz)
+		return -EINVAL;
+
+	/* sanity checks */
+	if (ucore->inlen > sizeof(cmd) &&
+	    !ib_is_udata_cleared(ucore, sizeof(cmd),
+				 ucore->inlen - sizeof(cmd)))
+		return -EOPNOTSUPP;
+
+	ret = ib_copy_from_udata(&cmd, ucore, min(sizeof(cmd), ucore->inlen));
+	if (ret)
+		return ret;
+
+	if (!cmd.attr_mask || cmd.reserved)
+		return -EINVAL;
+
+	if (cmd.attr_mask > IB_CQ_MODERATE)
+		return -EOPNOTSUPP;
+
+	cq = uobj_get_obj_read(cq, cmd.cq_handle, file->ucontext);
+	if (!cq)
+		return -EINVAL;
+
+	ret = ib_modify_cq(cq, cmd.attr.cq_count, cmd.attr.cq_period);
+
+	uobj_put_obj_read(cq);
+
+	return ret;
+}
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index b5febfd84ee5..381fd9c096ae 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -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);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 0b671982bbb3..8e0d3780ce4e 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -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;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index d4e0b53bfc75..cfa09d6095d6 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 2/6] IB/mlx4: Exposing modify CQ callback to uverbs layer

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Exposed mlx4_ib_modify_cq to be called from ib device
verb list.

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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(+)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index c636842c5be0..58845e67dad1 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -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) ||
diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
index 1fa19820355a..9a55064ed69a 100644
--- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
+++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
@@ -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"
 
diff --git a/include/linux/mlx4/cq.h b/include/linux/mlx4/cq.h
index 09cebe528488..508e8cc5ee86 100644
--- a/include/linux/mlx4/cq.h
+++ b/include/linux/mlx4/cq.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 3/6] IB/mlx5: Exposing modify CQ callback to uverbs layer

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Exposed mlx5_ib_modify_cq to be called from ib device
verb list.

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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(-)

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index 01b218a3c277..18705cbcdc8c 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -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)
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index e5f8d5d1cd7e..838c47012388 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -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;
diff --git a/include/linux/mlx5/cq.h b/include/linux/mlx5/cq.h
index cc718e245b1e..6be357b219ec 100644
--- a/include/linux/mlx5/cq.h
+++ b/include/linux/mlx5/cq.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 4/6] IB/uverbs: Add CQ moderation capability to query_device

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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.

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 3c2673cd4090..53143e4b1c50 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -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;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 8e0d3780ce4e..0b484c023fa9 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -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 {
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index cfa09d6095d6..5186fb12629b 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 5/6] IB/mlx4: Add CQ moderation capability to query_device

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 58845e67dad1..c6420e26f2d8 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next v1 6/6] IB/mlx5: Add CQ moderation capability to query_device

[Leon Romanovsky]

From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

Signed-off-by: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 838c47012388..18fe01826942 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Sun, Oct 29, 2017 at 03:51:35PM +0200, Leon Romanovsky wrote:
> From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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.


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?

> +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> +			   struct ib_device *ib_dev,
> +			   struct ib_udata *ucore,
> +			   struct ib_udata *uhw)

Is this really a good idea?

Why not ib_uverbs_set_cq_moderation ?

We don't have to keep making these thicker and thicker APIs if there
isn't a reason :(

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-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

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

On Sun, Oct 29, 2017 at 11:43:45AM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 29, 2017 at 03:51:35PM +0200, Leon Romanovsky wrote:
> > From: Yonatan Cohen <yonatanc-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > 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.
>
>
> Like the other recent uAPI patches, lets us see the rdma-core side and
> man page first please.

Yishai planned to send it tomorrow.

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

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

>
> I guess it is for UD applications where the existing signalled stuff
> isn't applicable?
>
> > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > +			   struct ib_device *ib_dev,
> > +			   struct ib_udata *ucore,
> > +			   struct ib_udata *uhw)
>
> Is this really a good idea?
>
> Why not ib_uverbs_set_cq_moderation ?

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

>
> We don't have to keep making these thicker and thicker APIs if there
> isn't a reason :(
>
> Jason

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

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Sun, Oct 29, 2017 at 08:28:08PM +0200, Leon Romanovsky wrote:

> > > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > > +			   struct ib_device *ib_dev,
> > > +			   struct ib_udata *ucore,
> > > +			   struct ib_udata *uhw)
> >
> > Is this really a good idea?
> >
> > Why not ib_uverbs_set_cq_moderation ?
> 
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

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

On Mon, Oct 30, 2017 at 08:48:07AM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 29, 2017 at 08:28:08PM +0200, Leon Romanovsky wrote:
>
> > > > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > > > +			   struct ib_device *ib_dev,
> > > > +			   struct ib_udata *ucore,
> > > > +			   struct ib_udata *uhw)
> > >
> > > Is this really a good idea?
> > >
> > > Why not ib_uverbs_set_cq_moderation ?
> >
> > 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);

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

>
> Jason

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

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Mon, Oct 30, 2017 at 05:28:15PM +0200, Leon Romanovsky wrote:
> > + * 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);
> 
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

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

On Mon, Oct 30, 2017 at 09:52:36AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 30, 2017 at 05:28:15PM +0200, Leon Romanovsky wrote:
> > > + * 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);
> >
> > 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.
>
> 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..

"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

>
> Jason

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

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Mon, Oct 30, 2017 at 09:09:52PM +0200, Leon Romanovsky wrote:

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

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

On Mon, Oct 30, 2017 at 05:07:53PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 30, 2017 at 09:09:52PM +0200, Leon Romanovsky wrote:
>
> > 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.
>
> 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.

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.

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

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

>
> Jason

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

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Yishai Hadas]

On 10/31/2017 7:08 AM, Leon Romanovsky wrote:
> On Mon, Oct 30, 2017 at 05:07:53PM -0600, Jason Gunthorpe wrote:
>> On Mon, Oct 30, 2017 at 09:09:52PM +0200, Leon Romanovsky wrote:
>>
>>> 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.
>>
>> 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.
> 
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Tue, Oct 31, 2017 at 01:31:28PM +0200, Yishai Hadas wrote:
> On 10/31/2017 7:08 AM, Leon Romanovsky wrote:
> >On Mon, Oct 30, 2017 at 05:07:53PM -0600, Jason Gunthorpe wrote:
> >>On Mon, Oct 30, 2017 at 09:09:52PM +0200, Leon Romanovsky wrote:
> >>
> >>>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.
> >>
> >>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.
> >
> >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.
> 
> Even for the user area having a dedicated 'setter' per field might bring
> some redundant overhead and limitation.

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.

> This will prevent the option from application to modify few fields
> in one API and resulted in a system call per field.

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.

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Yishai Hadas]

On 10/31/2017 5:40 PM, Jason Gunthorpe wrote:
> On Tue, Oct 31, 2017 at 01:31:28PM +0200, Yishai Hadas wrote:
>> On 10/31/2017 7:08 AM, Leon Romanovsky wrote:
>>> On Mon, Oct 30, 2017 at 05:07:53PM -0600, Jason Gunthorpe wrote:
>>>> On Mon, Oct 30, 2017 at 09:09:52PM +0200, Leon Romanovsky wrote:
>>>>
>>>>> 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.
>>>>
>>>> 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.
>>>
>>> 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.
>>
>> 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.
> 
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Tue, Oct 31, 2017 at 07:06:05PM +0200, Yishai Hadas wrote:

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

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

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Yishai Hadas]

On 11/1/2017 8:06 PM, Jason Gunthorpe wrote:
> On Tue, Oct 31, 2017 at 07:06:05PM +0200, Yishai Hadas wrote:
> 
>> 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.
> 
> Yep, those perhaps should not have used the attr_mask scheme either
> 

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

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Doug Ledford]

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

On Mon, 2017-10-30 at 17:28 +0200, Leon Romanovsky wrote:
> On Mon, Oct 30, 2017 at 08:48:07AM -0600, Jason Gunthorpe wrote:
> > On Sun, Oct 29, 2017 at 08:28:08PM +0200, Leon Romanovsky wrote:
> > 
> > > > > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > > > > +			   struct ib_device *ib_dev,
> > > > > +			   struct ib_udata *ucore,
> > > > > +			   struct ib_udata *uhw)
> > > > 
> > > > Is this really a good idea?
> > > > 
> > > > Why not ib_uverbs_set_cq_moderation ?
> > > 
> > > 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);
> 
> 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Fri, Nov 10, 2017 at 02:15:29PM -0500, Doug Ledford wrote:

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

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.

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Leon Romanovsky]

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

On Fri, Nov 10, 2017 at 02:15:29PM -0500, Doug Ledford wrote:
> On Mon, 2017-10-30 at 17:28 +0200, Leon Romanovsky wrote:
> > On Mon, Oct 30, 2017 at 08:48:07AM -0600, Jason Gunthorpe wrote:
> > > On Sun, Oct 29, 2017 at 08:28:08PM +0200, Leon Romanovsky wrote:
> > >
> > > > > > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > > > > > +			   struct ib_device *ib_dev,
> > > > > > +			   struct ib_udata *ucore,
> > > > > > +			   struct ib_udata *uhw)
> > > > >
> > > > > Is this really a good idea?
> > > > >
> > > > > Why not ib_uverbs_set_cq_moderation ?
> > > >
> > > > 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);
> >
> > 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.
>
> 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.

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

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

Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Jason Gunthorpe]

On Sat, Nov 11, 2017 at 10:09:43AM +0200, Leon Romanovsky wrote:

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html