diff mbox series

[rdma-core,04/13] verbs: Handle async FD on an imported device

Message ID 1592379956-7043-5-git-send-email-yishaih@mellanox.com (mailing list archive)
State Not Applicable
Headers show
Series verbs: Introduce import verbs for device, PD, MR | expand

Commit Message

Yishai Hadas June 17, 2020, 7:45 a.m. UTC 
An imported device needs to allocate its own async FD to be able to get
events on its private created objects.

This patch use ibv_cmd_alloc_async_fd() to achieve that by forcing any
applicable event's object (e.g QP, SRC, etc.) upon its creation to use
the ioctl interface as now the context async FD becomes a mandatory
attribute.

From symmetric reasons we moved ibv_cmd_alloc_async_fd() to be called as
part of ibv_open_device() in case wasn't allocated yet (i.e. ioctl flow).

Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
---
 libibverbs/cmd_cq.c     |  9 +++++++--
 libibverbs/cmd_device.c |  5 +----
 libibverbs/cmd_qp.c     |  4 ++++
 libibverbs/cmd_srq.c    |  4 ++++
 libibverbs/cmd_wq.c     |  4 ++++
 libibverbs/device.c     | 15 +++++++++++++++
 libibverbs/driver.h     |  1 +
 libibverbs/ibverbs.h    |  1 +
 8 files changed, 37 insertions(+), 6 deletions(-)

Comments

Jason Gunthorpe June 19, 2020, 12:33 p.m. UTC | #1 
On Wed, Jun 17, 2020 at 10:45:47AM +0300, Yishai Hadas wrote:
> @@ -418,7 +427,13 @@ static struct ibv_context *verbs_import_device(int cmd_fd)
>  
>  	set_lib_ops(context_ex);
>  
> +	context_ex->priv->imported = true;
>  	ctx = &context_ex->context;
> +	ret = ibv_cmd_alloc_async_fd(ctx);
> +	if (ret) {
> +		ibv_close_device(ctx);
> +		ctx = NULL;
> +	}
>  out:
>  	ibv_free_device_list(dev_list);
>  	return ctx;

This hunk should probably be in the prior patch, or ideally the order
of these two patches should be swapped.

Jason
Yishai Hadas June 21, 2020, 9:08 a.m. UTC | #2 
On 6/19/2020 3:33 PM, Jason Gunthorpe wrote:
> On Wed, Jun 17, 2020 at 10:45:47AM +0300, Yishai Hadas wrote:
>> @@ -418,7 +427,13 @@ static struct ibv_context *verbs_import_device(int cmd_fd)
>>   
>>   	set_lib_ops(context_ex);
>>   
>> +	context_ex->priv->imported = true;
>>   	ctx = &context_ex->context;
>> +	ret = ibv_cmd_alloc_async_fd(ctx);
>> +	if (ret) {
>> +		ibv_close_device(ctx);
>> +		ctx = NULL;
>> +	}
>>   out:
>>   	ibv_free_device_list(dev_list);
>>   	return ctx;
> 
> This hunk should probably be in the prior patch, or ideally the order
> of these two patches should be swapped.
> 

I can swap the order of those two patches as you suggested.
However, in this case, the first one will be some preparation to force 
the ioctl mode upon an 'imported' flow and only then the second one will 
introduce the 'imported' flow as part of adding ibv_import_device().
If you are fine with that let's go by that approach.

Yishai
Jason Gunthorpe June 23, 2020, 5:34 p.m. UTC | #3 
On Sun, Jun 21, 2020 at 12:08:57PM +0300, Yishai Hadas wrote:
> On 6/19/2020 3:33 PM, Jason Gunthorpe wrote:
> > On Wed, Jun 17, 2020 at 10:45:47AM +0300, Yishai Hadas wrote:
> > > @@ -418,7 +427,13 @@ static struct ibv_context *verbs_import_device(int cmd_fd)
> > >   	set_lib_ops(context_ex);
> > > +	context_ex->priv->imported = true;
> > >   	ctx = &context_ex->context;
> > > +	ret = ibv_cmd_alloc_async_fd(ctx);
> > > +	if (ret) {
> > > +		ibv_close_device(ctx);
> > > +		ctx = NULL;
> > > +	}
> > >   out:
> > >   	ibv_free_device_list(dev_list);
> > >   	return ctx;
> > 
> > This hunk should probably be in the prior patch, or ideally the order
> > of these two patches should be swapped.
> > 
> 
> I can swap the order of those two patches as you suggested.
> However, in this case, the first one will be some preparation to force the
> ioctl mode upon an 'imported' flow and only then the second one will
> introduce the 'imported' flow as part of adding ibv_import_device().
> If you are fine with that let's go by that approach.

prep patches are better than having patches with incomplete
functionality

Jason
Yishai Hadas June 24, 2020, 7:22 a.m. UTC | #4 
On 6/23/2020 8:34 PM, Jason Gunthorpe wrote:
> On Sun, Jun 21, 2020 at 12:08:57PM +0300, Yishai Hadas wrote:
>> On 6/19/2020 3:33 PM, Jason Gunthorpe wrote:
>>> On Wed, Jun 17, 2020 at 10:45:47AM +0300, Yishai Hadas wrote:
>>>> @@ -418,7 +427,13 @@ static struct ibv_context *verbs_import_device(int cmd_fd)
>>>>    	set_lib_ops(context_ex);
>>>> +	context_ex->priv->imported = true;
>>>>    	ctx = &context_ex->context;
>>>> +	ret = ibv_cmd_alloc_async_fd(ctx);
>>>> +	if (ret) {
>>>> +		ibv_close_device(ctx);
>>>> +		ctx = NULL;
>>>> +	}
>>>>    out:
>>>>    	ibv_free_device_list(dev_list);
>>>>    	return ctx;
>>>
>>> This hunk should probably be in the prior patch, or ideally the order
>>> of these two patches should be swapped.
>>>
>>
>> I can swap the order of those two patches as you suggested.
>> However, in this case, the first one will be some preparation to force the
>> ioctl mode upon an 'imported' flow and only then the second one will
>> introduce the 'imported' flow as part of adding ibv_import_device().
>> If you are fine with that let's go by that approach.
> 
> prep patches are better than having patches with incomplete
> functionality
> 

Agree, already was updated in the PR accordingly.

Yishai
diff mbox series

Patch

diff --git a/libibverbs/cmd_cq.c b/libibverbs/cmd_cq.c
index 1ac5ca5..5412660 100644
--- a/libibverbs/cmd_cq.c
+++ b/libibverbs/cmd_cq.c
@@ -31,6 +31,7 @@ 
  */
 
 #include <infiniband/cmd_write.h>
+#include "ibverbs.h"
 
 static int ibv_icmd_create_cq(struct ibv_context *context, int cqe,
 			      struct ibv_comp_channel *channel, int comp_vector,
@@ -38,6 +39,7 @@  static int ibv_icmd_create_cq(struct ibv_context *context, int cqe,
 			      struct ibv_command_buffer *link)
 {
 	DECLARE_FBCMD_BUFFER(cmdb, UVERBS_OBJECT_CQ, UVERBS_METHOD_CQ_CREATE, 8, link);
+	struct verbs_ex_private *priv = get_priv(context);
 	struct ib_uverbs_attr *handle;
 	struct ib_uverbs_attr *async_fd_attr;
 	uint32_t resp_cqe;
@@ -54,8 +56,11 @@  static int ibv_icmd_create_cq(struct ibv_context *context, int cqe,
 		fill_attr_in_fd(cmdb, UVERBS_ATTR_CREATE_CQ_COMP_CHANNEL, channel->fd);
 	fill_attr_in_uint32(cmdb, UVERBS_ATTR_CREATE_CQ_COMP_VECTOR, comp_vector);
 	async_fd_attr = fill_attr_in_fd(cmdb, UVERBS_ATTR_CREATE_CQ_EVENT_FD, context->async_fd);
-	/* Prevent fallback to the 'write' mode if kernel doesn't support it */
-	attr_optional(async_fd_attr);
+	if (priv->imported)
+		fallback_require_ioctl(cmdb);
+	else
+		/* Prevent fallback to the 'write' mode if kernel doesn't support it */
+		attr_optional(async_fd_attr);
 
 	if (flags) {
 		fallback_require_ex(cmdb);
diff --git a/libibverbs/cmd_device.c b/libibverbs/cmd_device.c
index 4de59c0..648cc0b 100644
--- a/libibverbs/cmd_device.c
+++ b/libibverbs/cmd_device.c
@@ -99,7 +99,7 @@  int ibv_cmd_query_port(struct ibv_context *context, uint8_t port_num,
 	return 0;
 }
 
-static int cmd_alloc_async_fd(struct ibv_context *context)
+int ibv_cmd_alloc_async_fd(struct ibv_context *context)
 {
 	DECLARE_COMMAND_BUFFER(cmdb, UVERBS_OBJECT_ASYNC_EVENT,
 			       UVERBS_METHOD_ASYNC_EVENT_ALLOC, 1);
@@ -153,9 +153,6 @@  static int cmd_get_context(struct verbs_context *context_ex,
 		return 0;
 	}
 	case SUCCESS:
-		ret = cmd_alloc_async_fd(context);
-		if (ret)
-			return ret;
 		break;
 	default:
 		return ret;
diff --git a/libibverbs/cmd_qp.c b/libibverbs/cmd_qp.c
index a11bb98..567984d 100644
--- a/libibverbs/cmd_qp.c
+++ b/libibverbs/cmd_qp.c
@@ -76,6 +76,7 @@  static int ibv_icmd_create_qp(struct ibv_context *context,
 			      struct ibv_command_buffer *link)
 {
 	DECLARE_FBCMD_BUFFER(cmdb, UVERBS_OBJECT_QP, UVERBS_METHOD_QP_CREATE, 15, link);
+	struct verbs_ex_private *priv = get_priv(context);
 	struct ib_uverbs_attr *handle;
 	uint32_t qp_num;
 	uint32_t pd_handle;
@@ -172,6 +173,9 @@  static int ibv_icmd_create_qp(struct ibv_context *context,
 	fill_attr_in_ptr(cmdb, UVERBS_ATTR_CREATE_QP_CAP, &attr_ex->cap);
 	fill_attr_in_fd(cmdb, UVERBS_ATTR_CREATE_QP_EVENT_FD, context->async_fd);
 
+	if (priv->imported)
+		fallback_require_ioctl(cmdb);
+
 	if (attr_ex->sq_sig_all)
 		create_flags |= IB_UVERBS_QP_CREATE_SQ_SIG_ALL;
 
diff --git a/libibverbs/cmd_srq.c b/libibverbs/cmd_srq.c
index 4e63046..dfaaa6a 100644
--- a/libibverbs/cmd_srq.c
+++ b/libibverbs/cmd_srq.c
@@ -53,6 +53,7 @@  static int ibv_icmd_create_srq(struct ibv_pd *pd, struct verbs_srq *vsrq,
 			       struct ibv_command_buffer *link)
 {
 	DECLARE_FBCMD_BUFFER(cmdb, UVERBS_OBJECT_SRQ, UVERBS_METHOD_SRQ_CREATE, 13, link);
+	struct verbs_ex_private *priv = get_priv(pd->context);
 	struct ib_uverbs_attr *handle;
 	uint32_t max_wr;
 	uint32_t max_sge;
@@ -107,6 +108,9 @@  static int ibv_icmd_create_srq(struct ibv_pd *pd, struct verbs_srq *vsrq,
 	fill_attr_out_ptr(cmdb, UVERBS_ATTR_CREATE_SRQ_RESP_MAX_WR, &max_wr);
 	fill_attr_out_ptr(cmdb, UVERBS_ATTR_CREATE_SRQ_RESP_MAX_SGE, &max_sge);
 
+	if (priv->imported)
+		fallback_require_ioctl(cmdb);
+
 	switch (execute_ioctl_fallback(srq->context, create_srq, cmdb, &ret)) {
 	case TRY_WRITE: {
 		if (attr_ex->srq_type == IBV_SRQT_BASIC && abi_ver > 5) {
diff --git a/libibverbs/cmd_wq.c b/libibverbs/cmd_wq.c
index d233c3a..855fc04 100644
--- a/libibverbs/cmd_wq.c
+++ b/libibverbs/cmd_wq.c
@@ -31,6 +31,7 @@ 
  */
 
 #include <infiniband/cmd_write.h>
+#include "ibverbs.h"
 
 static int ibv_icmd_create_wq(struct ibv_context *context,
 			      struct ibv_wq_init_attr *wq_init_attr,
@@ -38,6 +39,7 @@  static int ibv_icmd_create_wq(struct ibv_context *context,
 			      struct ibv_command_buffer *link)
 {
 	DECLARE_FBCMD_BUFFER(cmdb, UVERBS_OBJECT_WQ, UVERBS_METHOD_WQ_CREATE, 13, link);
+	struct verbs_ex_private *priv = get_priv(context);
 	struct ib_uverbs_attr *handle;
 	uint32_t max_wr;
 	uint32_t max_sge;
@@ -62,6 +64,8 @@  static int ibv_icmd_create_wq(struct ibv_context *context,
 	fill_attr_out_ptr(cmdb, UVERBS_ATTR_CREATE_WQ_RESP_MAX_SGE, &max_sge);
 	fill_attr_out_ptr(cmdb, UVERBS_ATTR_CREATE_WQ_RESP_WQ_NUM, &wq_num);
 
+	if (priv->imported)
+		fallback_require_ioctl(cmdb);
 	fallback_require_ex(cmdb);
 
 	switch (execute_ioctl_fallback(context, create_wq, cmdb, &ret)) {
diff --git a/libibverbs/device.c b/libibverbs/device.c
index 0a8403d..595af82 100644
--- a/libibverbs/device.c
+++ b/libibverbs/device.c
@@ -344,6 +344,7 @@  struct ibv_context *verbs_open_device(struct ibv_device *device, void *private_d
 	struct verbs_device *verbs_device = verbs_get_device(device);
 	int cmd_fd;
 	struct verbs_context *context_ex;
+	int ret;
 
 	/*
 	 * We'll only be doing writes, but we need O_RDWR in case the
@@ -363,6 +364,13 @@  struct ibv_context *verbs_open_device(struct ibv_device *device, void *private_d
 		return NULL;
 
 	set_lib_ops(context_ex);
+	if (context_ex->context.async_fd == -1) {
+		ret = ibv_cmd_alloc_async_fd(&context_ex->context);
+		if (ret) {
+			ibv_close_device(&context_ex->context);
+			return NULL;
+		}
+	}
 
 	return &context_ex->context;
 }
@@ -381,6 +389,7 @@  static struct ibv_context *verbs_import_device(int cmd_fd)
 	struct ibv_device **dev_list;
 	struct ibv_context *ctx = NULL;
 	struct stat st;
+	int ret;
 	int i;
 
 	if (fstat(cmd_fd, &st) || !S_ISCHR(st.st_mode)) {
@@ -418,7 +427,13 @@  static struct ibv_context *verbs_import_device(int cmd_fd)
 
 	set_lib_ops(context_ex);
 
+	context_ex->priv->imported = true;
 	ctx = &context_ex->context;
+	ret = ibv_cmd_alloc_async_fd(ctx);
+	if (ret) {
+		ibv_close_device(ctx);
+		ctx = NULL;
+	}
 out:
 	ibv_free_device_list(dev_list);
 	return ctx;
diff --git a/libibverbs/driver.h b/libibverbs/driver.h
index 52ecbfe..98a1b24 100644
--- a/libibverbs/driver.h
+++ b/libibverbs/driver.h
@@ -446,6 +446,7 @@  int ibv_cmd_query_device_ex(struct ibv_context *context,
 int ibv_cmd_query_port(struct ibv_context *context, uint8_t port_num,
 		       struct ibv_port_attr *port_attr,
 		       struct ibv_query_port *cmd, size_t cmd_size);
+int ibv_cmd_alloc_async_fd(struct ibv_context *context);
 int ibv_cmd_alloc_pd(struct ibv_context *context, struct ibv_pd *pd,
 		     struct ibv_alloc_pd *cmd, size_t cmd_size,
 		     struct ib_uverbs_alloc_pd_resp *resp, size_t resp_size);
diff --git a/libibverbs/ibverbs.h b/libibverbs/ibverbs.h
index 4b9b88f..6703c10 100644
--- a/libibverbs/ibverbs.h
+++ b/libibverbs/ibverbs.h
@@ -74,6 +74,7 @@  struct verbs_ex_private {
 	uint32_t driver_id;
 	bool use_ioctl_write;
 	struct verbs_context_ops ops;
+	bool imported;
 };
 
 static inline struct verbs_ex_private *get_priv(struct ibv_context *ctx)