[PATCH v1 ib-next 0/3] IB core uverbs support for leagacy commands

[PATCH v1 ib-next 0/3] IB core uverbs support for leagacy commands

[Eli Cohen]

Hi Doug,

this patcheset is addresses comments from the community. Specifically if the verbs is not supported by a hardware driver, we return -EOPNOTSUPP.

Eli

Eli Cohen (3):
  IB/core: Avoid duplicate code
  IB/core: IB/core: Allow legacy verbs through extended interfaces
  IB/core: Modify conditional on ucontext existence

 drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

-- 
2.6.3

--
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 v1 ib-next 1/3] IB/core: Avoid duplicate code

[Eli Cohen]

Move the check on the validity of the command to a common area.

Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e3ef28861be6..e93ba9125198 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -678,6 +678,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_device *ib_dev;
 	struct ib_uverbs_cmd_hdr hdr;
+	__u32 command;
 	__u32 flags;
 	int srcu_key;
 	ssize_t ret;
@@ -696,20 +697,18 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
+	if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+				   IB_USER_VERBS_CMD_COMMAND_MASK)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
 	if (!flags) {
-		__u32 command;
-
-		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
-					   IB_USER_VERBS_CMD_COMMAND_MASK)) {
-			ret = -EINVAL;
-			goto out;
-		}
-
-		command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
-
 		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
 		    !uverbs_cmd_table[command]) {
 			ret = -EINVAL;
@@ -738,21 +737,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						 hdr.out_words * 4);
 
 	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
-		__u32 command;
-
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
-					   IB_USER_VERBS_CMD_COMMAND_MASK)) {
-			ret = -EINVAL;
-			goto out;
-		}
-
-		command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
-
 		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
 		    !uverbs_ex_cmd_table[command]) {
 			ret = -ENOSYS;
-- 
2.6.3

--
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 v1 ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

When an extended verbs is an extension to a legacy verb, the original
functionality is preserved. Hence we do not require each hardware driver
to set the extended capability. This will allow to use the extended verb
in its simple form with drivers that do not support the extended
capability.

Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

Changes from previous version:
If verify_command_mask fails return -EOPNOTSUPP.

 drivers/infiniband/core/uverbs_main.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e93ba9125198..ecb907385b85 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -672,6 +672,21 @@ out:
 	return ev_file;
 }
 
+static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
+{
+	u64 mask;
+
+	if (command <= IB_USER_VERBS_CMD_OPEN_QP)
+		mask = ib_dev->uverbs_cmd_mask;
+	else
+		mask = ib_dev->uverbs_ex_cmd_mask;
+
+	if (mask & ((u64)1 << command))
+		return 0;
+
+	return -1;
+}
+
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			     size_t count, loff_t *pos)
 {
@@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	}
 
 	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+	if (verify_command_mask(ib_dev, command)) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
 
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
@@ -721,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!(ib_dev->uverbs_cmd_mask & (1ull << command))) {
-			ret = -ENOSYS;
-			goto out;
-		}
-
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
@@ -753,11 +767,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
-			ret = -ENOSYS;
-			goto out;
-		}
-
 		if (count < (sizeof(hdr) + sizeof(ex_hdr))) {
 			ret = -EINVAL;
 			goto out;
-- 
2.6.3

--
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 v1 ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 08:00:09PM +0200, Eli Cohen wrote:
> When an extended verbs is an extension to a legacy verb, the original
> functionality is preserved. Hence we do not require each hardware driver
> to set the extended capability. This will allow to use the extended verb
> in its simple form with drivers that do not support the extended
> capability.

Can we please get rid of uverbs_cmd_mask and uverbs_ex_cmd_mask ?

The driver should set the function pointers it does not support to
NULL. We don't need the bitmask because we already know the answer.

For performance, I recommend having the core replace the NULL op
pointers from the driver with a stub that returns ENOTSUP, and then
drop all the checking in the fast path.

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 v1 ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

On Tue, Nov 10, 2015 at 11:21:07AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 10, 2015 at 08:00:09PM +0200, Eli Cohen wrote:
> > When an extended verbs is an extension to a legacy verb, the original
> > functionality is preserved. Hence we do not require each hardware driver
> > to set the extended capability. This will allow to use the extended verb
> > in its simple form with drivers that do not support the extended
> > capability.
> 
> Can we please get rid of uverbs_cmd_mask and uverbs_ex_cmd_mask ?
> 
> The driver should set the function pointers it does not support to
> NULL. We don't need the bitmask because we already know the answer.
> 
> For performance, I recommend having the core replace the NULL op
> pointers from the driver with a stub that returns ENOTSUP, and then
> drop all the checking in the fast path.
>

Yes, we can do this but I think this should be the subject for another
patch, agree?

Regarding using stabs, it may be nice but I don't think performance is
the issue here. Most verbs implementations involve relatively long i/o
operations anyway so the gain will not be noticable.
--
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 v1 ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 09:57:13PM +0200, Eli Cohen wrote:

> Yes, we can do this but I think this should be the subject for another
> patch, agree?

Sure

> Regarding using stabs, it may be nice but I don't think performance is
> the issue here. Most verbs implementations involve relatively long i/o
> operations anyway so the gain will not be noticable.

Intel keeps optimizing this common path since they use it for post..

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

[PATCH v1 ib-next 3/3] IB/core: Modify conditional on ucontext existence

[Eli Cohen]

Since we allow to call legacy verbs using their extended counterpart,
the check on ucontext has to move up to a common area in case this verb
is ever extended.

Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index ecb907385b85..5a0416050074 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -724,6 +724,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
+	if (!file->ucontext &&
+	    command != IB_USER_VERBS_CMD_GET_CONTEXT) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
@@ -734,12 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!file->ucontext &&
-		    command != IB_USER_VERBS_CMD_GET_CONTEXT) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
-- 
2.6.3

--
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 v1 ib-next 0/3] IB core uverbs support for leagacy commands

[Leon Romanovsky]

On Tue, Nov 10, 2015 at 08:00:07PM +0200, Eli Cohen wrote:
> Hi Doug,
> 
> this patcheset is addresses comments from the community. Specifically if the verbs is not supported by a hardware driver, we return -EOPNOTSUPP.
> 
> Eli
> 
> Eli Cohen (3):
>   IB/core: Avoid duplicate code
>   IB/core: IB/core: Allow legacy verbs through extended interfaces
>   IB/core: Modify conditional on ucontext existence
> 
>  drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 36 deletions(-)

Doug,
I remember that you experienced the issues with your email setup and I
wonder if this patchset was left behind.
https://patchwork.kernel.org/patch/7591731/
https://patchwork.kernel.org/patch/7591741/
https://patchwork.kernel.org/patch/7591751/

It is important patchset which enables create_qp_ex() interface
for already merged patches.

Thanks.

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