linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] libibverbs: Add I/O memory registration verbs
@ 2010-07-29 16:31 Tom Tucker
       [not found] ` <20100729163030.14901.87547.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Tom Tucker @ 2010-07-29 16:31 UTC (permalink / raw)
  To: rdreier-FYB4Gu1CFyUAvxtiuMwx3w
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, brandt-4OHPYypu0djtX7QSmKvirg,
	tom-/Yg/VP3ZvrM, swise-/Yg/VP3ZvrM

This patch series adds support for the new I/O memory registration services
to libibverbs.

---

Tom Tucker (3):
      libibverbs: Add reg/unreg I/O memory verbs
      libibverbs: Add reg/unreg I/O memory commands to kern ABI
      libibverbs: Fix libtool configure warning


 configure.in                  |    1 +
 include/infiniband/driver.h   |    6 ++++++
 include/infiniband/kern-abi.h |   32 +++++++++++++++++++++++++++++++-
 include/infiniband/verbs.h    |   14 ++++++++++++++
 src/cmd.c                     |   41 +++++++++++++++++++++++++++++++++++++++++
 src/libibverbs.map            |    5 +++++
 src/verbs.c                   |   35 +++++++++++++++++++++++++++++++++++
 7 files changed, 133 insertions(+), 1 deletions(-)

-- 
Signed-off-by: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
--
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 1/3] libibverbs: Fix libtool configure warning
       [not found] ` <20100729163030.14901.87547.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
@ 2010-07-29 16:31   ` Tom Tucker
  2010-07-29 16:31   ` [RFC PATCH 2/3] libibverbs: Add reg/unreg I/O memory commands to kern ABI Tom Tucker
  2010-07-29 16:32   ` [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs Tom Tucker
  2 siblings, 0 replies; 7+ messages in thread
From: Tom Tucker @ 2010-07-29 16:31 UTC (permalink / raw)
  To: rdreier-FYB4Gu1CFyUAvxtiuMwx3w
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, brandt-4OHPYypu0djtX7QSmKvirg,
	tom-/Yg/VP3ZvrM, swise-/Yg/VP3ZvrM

From: Tom Tucker <tom-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

Added AC_PROG_LIBTOOL to configure.in to fix an autogen.sh warning about
LIBTOOL configuration.

Signed-off-by: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
---

 configure.in |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure.in b/configure.in
index 927c406..2bb4aee 100644
--- a/configure.in
+++ b/configure.in
@@ -12,6 +12,7 @@ dnl Checks for programs
 AC_PROG_CC
 AC_GNU_SOURCE
 AC_PROG_LN_S
+AC_PROG_LIBTOOL
 
 LT_INIT
 

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC PATCH 2/3] libibverbs: Add reg/unreg I/O memory commands to kern ABI
       [not found] ` <20100729163030.14901.87547.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
  2010-07-29 16:31   ` [RFC PATCH 1/3] libibverbs: Fix libtool configure warning Tom Tucker
@ 2010-07-29 16:31   ` Tom Tucker
  2010-07-29 16:32   ` [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs Tom Tucker
  2 siblings, 0 replies; 7+ messages in thread
From: Tom Tucker @ 2010-07-29 16:31 UTC (permalink / raw)
  To: rdreier-FYB4Gu1CFyUAvxtiuMwx3w
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, brandt-4OHPYypu0djtX7QSmKvirg,
	tom-/Yg/VP3ZvrM, swise-/Yg/VP3ZvrM

From: Tom Tucker <tom-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

Add the commands to register and unregister I/O memory to the Kernel ABI.

Signed-off-by: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
---

 include/infiniband/kern-abi.h |   32 +++++++++++++++++++++++++++++++-
 src/cmd.c                     |   41 +++++++++++++++++++++++++++++++++++++++++
 src/libibverbs.map            |    5 +++++
 3 files changed, 77 insertions(+), 1 deletions(-)

diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
index 0db083a..56b538f 100644
--- a/include/infiniband/kern-abi.h
+++ b/include/infiniband/kern-abi.h
@@ -85,7 +85,9 @@ enum {
 	IB_USER_VERBS_CMD_MODIFY_SRQ,
 	IB_USER_VERBS_CMD_QUERY_SRQ,
 	IB_USER_VERBS_CMD_DESTROY_SRQ,
-	IB_USER_VERBS_CMD_POST_SRQ_RECV
+	IB_USER_VERBS_CMD_POST_SRQ_RECV,
+	IB_USER_VERBS_CMD_REG_IO_MR,
+	IB_USER_VERBS_CMD_DEREG_IO_MR,
 };
 
 /*
@@ -271,6 +273,32 @@ struct ibv_dereg_mr {
 	__u32 mr_handle;
 };
 
+struct ibv_reg_io_mr {
+        __u32 command;
+        __u16 in_words;
+        __u16 out_words;
+        __u64 response;
+        __u64 start;
+        __u64 length;
+        __u64 hca_va;
+        __u32 pd_handle;
+        __u32 access_flags;
+        __u64 driver_data[0];
+};
+
+struct ibv_reg_io_mr_resp {
+        __u32 mr_handle;
+        __u32 lkey;
+        __u32 rkey;
+};
+
+struct ibv_dereg_io_mr {
+        __u32 command;
+        __u16 in_words;
+        __u16 out_words;
+        __u32 mr_handle;
+};
+
 struct ibv_create_comp_channel {
 	__u32 command;
 	__u16 in_words;
@@ -803,6 +831,8 @@ enum {
 	 * trick opcodes in IBV_INIT_CMD() doesn't break.
 	 */
 	IB_USER_VERBS_CMD_CREATE_COMP_CHANNEL_V2 = -1,
+	IB_USER_VERBS_CMD_REG_IO_MR_V2 = -1,
+	IB_USER_VERBS_CMD_DEREG_IO_MR_V2 = -1,
 };
 
 struct ibv_destroy_cq_v1 {
diff --git a/src/cmd.c b/src/cmd.c
index cbd5288..f8fe6d5 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -271,6 +271,47 @@ int ibv_cmd_dereg_mr(struct ibv_mr *mr)
 	return 0;
 }
 
+int ibv_cmd_reg_io_mr(struct ibv_pd *pd, void *addr, size_t length,
+		      uint64_t hca_va, int access,
+		      struct ibv_mr *mr, struct ibv_reg_io_mr *cmd,
+		      size_t cmd_size,
+		      struct ibv_reg_io_mr_resp *resp, size_t resp_size)
+{
+
+	IBV_INIT_CMD_RESP(cmd, cmd_size, REG_IO_MR, resp, resp_size);
+
+	cmd->start 	  = (uintptr_t) addr;
+	cmd->length 	  = length;
+	cmd->hca_va 	  = hca_va;
+	cmd->pd_handle 	  = pd->handle;
+	cmd->access_flags = access;
+
+	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
+		return errno;
+
+	VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
+
+	mr->handle  = resp->mr_handle;
+	mr->lkey    = resp->lkey;
+	mr->rkey    = resp->rkey;
+	mr->context = pd->context;
+
+	return 0;
+}
+
+int ibv_cmd_dereg_io_mr(struct ibv_mr *mr)
+{
+	struct ibv_dereg_io_mr cmd;
+
+	IBV_INIT_CMD(&cmd, sizeof cmd, DEREG_IO_MR);
+	cmd.mr_handle = mr->handle;
+
+	if (write(mr->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
+		return errno;
+
+	return 0;
+}
+
 static int ibv_cmd_create_cq_v2(struct ibv_context *context, int cqe,
 				struct ibv_cq *cq,
 				struct ibv_create_cq *new_cmd, size_t new_cmd_size,
diff --git a/src/libibverbs.map b/src/libibverbs.map
index 1827da0..bc8a251 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -84,6 +84,11 @@ IBVERBS_1.1 {
 		ibv_open_device;
 		ibv_close_device;
 
+                ibv_reg_io_mr;
+                ibv_cmd_reg_io_mr;
+                ibv_dereg_io_mr;
+                ibv_cmd_dereg_io_mr;
+
 		ibv_init_ah_from_wc;
 		ibv_create_ah_from_wc;
 		ibv_copy_ah_attr_from_kern;

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs
       [not found] ` <20100729163030.14901.87547.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
  2010-07-29 16:31   ` [RFC PATCH 1/3] libibverbs: Fix libtool configure warning Tom Tucker
  2010-07-29 16:31   ` [RFC PATCH 2/3] libibverbs: Add reg/unreg I/O memory commands to kern ABI Tom Tucker
@ 2010-07-29 16:32   ` Tom Tucker
       [not found]     ` <20100729163202.14901.44211.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
  2 siblings, 1 reply; 7+ messages in thread
From: Tom Tucker @ 2010-07-29 16:32 UTC (permalink / raw)
  To: rdreier-FYB4Gu1CFyUAvxtiuMwx3w
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, brandt-4OHPYypu0djtX7QSmKvirg,
	tom-/Yg/VP3ZvrM, swise-/Yg/VP3ZvrM

From: Tom Tucker <tom-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

Add the ibv_reg_io_mr and ibv_dereg_io_mr verbs.

Signed-off-by: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
---

 include/infiniband/driver.h |    6 ++++++
 include/infiniband/verbs.h  |   14 ++++++++++++++
 src/verbs.c                 |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
index 9a81416..37c0ed1 100644
--- a/include/infiniband/driver.h
+++ b/include/infiniband/driver.h
@@ -82,6 +82,12 @@ int ibv_cmd_reg_mr(struct ibv_pd *pd, void *addr, size_t length,
 		   size_t cmd_size,
 		   struct ibv_reg_mr_resp *resp, size_t resp_size);
 int ibv_cmd_dereg_mr(struct ibv_mr *mr);
+int ibv_cmd_reg_io_mr(struct ibv_pd *pd, void *addr, size_t length,
+		      uint64_t hca_va, int access,
+		      struct ibv_mr *mr, struct ibv_reg_io_mr *cmd,
+		      size_t cmd_size,
+		      struct ibv_reg_io_mr_resp *resp, size_t resp_size);
+int ibv_cmd_dereg_io_mr(struct ibv_mr *mr);
 int ibv_cmd_create_cq(struct ibv_context *context, int cqe,
 		      struct ibv_comp_channel *channel,
 		      int comp_vector, struct ibv_cq *cq,
diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
index 0f1cb2e..a0d969a 100644
--- a/include/infiniband/verbs.h
+++ b/include/infiniband/verbs.h
@@ -640,6 +640,9 @@ struct ibv_context_ops {
 					    size_t length,
 					    int access);
 	int			(*dereg_mr)(struct ibv_mr *mr);
+        struct ibv_mr *         (*reg_io_mr)(struct ibv_pd *pd, void *addr, size_t length,
+					     int access);
+        int                     (*dereg_io_mr)(struct ibv_mr *mr);
 	struct ibv_mw *		(*alloc_mw)(struct ibv_pd *pd, enum ibv_mw_type type);
 	int			(*bind_mw)(struct ibv_qp *qp, struct ibv_mw *mw,
 					   struct ibv_mw_bind *mw_bind);
@@ -801,6 +804,17 @@ struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd, void *addr,
 int ibv_dereg_mr(struct ibv_mr *mr);
 
 /**
+ * ibv_reg_io_mr - Register a physical memory region
+ */
+struct ibv_mr *ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
+                             size_t length, int access);
+
+/**
+ * ibv_dereg_io_mr - Deregister a physical memory region
+ */
+int ibv_dereg_io_mr(struct ibv_mr *mr);
+
+/**
  * ibv_create_comp_channel - Create a completion event channel
  */
 struct ibv_comp_channel *ibv_create_comp_channel(struct ibv_context *context);
diff --git a/src/verbs.c b/src/verbs.c
index ba3c0a4..7d215c1 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -189,6 +189,41 @@ int __ibv_dereg_mr(struct ibv_mr *mr)
 }
 default_symver(__ibv_dereg_mr, ibv_dereg_mr);
 
+struct ibv_mr *__ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
+                               size_t length, int access)
+{
+        struct ibv_mr *mr;
+
+        if (ibv_dontfork_range(addr, length))
+                return NULL;
+
+        mr = pd->context->ops.reg_io_mr(pd, addr, length, access);
+        if (mr) {
+                mr->context = pd->context;
+                mr->pd      = pd;
+                mr->addr    = addr;
+                mr->length  = length;
+        } else
+                ibv_dofork_range(addr, length);
+
+        return mr;
+}
+default_symver(__ibv_reg_io_mr, ibv_reg_io_mr);
+
+int __ibv_dereg_io_mr(struct ibv_mr *mr)
+{
+        int ret;
+        void *addr      = mr->addr;
+        size_t length   = mr->length;
+
+        ret = mr->context->ops.dereg_io_mr(mr);
+        if (!ret)
+                ibv_dofork_range(addr, length);
+
+        return ret;
+}
+default_symver(__ibv_dereg_io_mr, ibv_dereg_io_mr);
+
 static struct ibv_comp_channel *ibv_create_comp_channel_v2(struct ibv_context *context)
 {
 	struct ibv_abi_compat_v2 *t = context->abi_compat;

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs
       [not found]     ` <20100729163202.14901.44211.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
@ 2010-07-29 20:07       ` Ralph Campbell
       [not found]         ` <1280434056.6820.136.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Ralph Campbell @ 2010-07-29 20:07 UTC (permalink / raw)
  To: Tom Tucker
  Cc: rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	brandt-4OHPYypu0djtX7QSmKvirg@public.gmane.org,
	swise-/Yg/VP3ZvrM@public.gmane.org

How does an application know when to call ibv_reg_io_mr()
instead of ibv_reg_mr()? It isn't going to know that some
address returned by mmap() is going to have the VM_PFNMAP
flag set.

How does an application know that the HCA supports
ibv_reg_io_mr() or not? (see below)
I think returning ENOTSUP or something would be good.

On Thu, 2010-07-29 at 09:32 -0700, Tom Tucker wrote:
> From: Tom Tucker <tom-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> 
> Add the ibv_reg_io_mr and ibv_dereg_io_mr verbs.
> 
> Signed-off-by: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
> ---
> 
>  include/infiniband/driver.h |    6 ++++++
>  include/infiniband/verbs.h  |   14 ++++++++++++++
>  src/verbs.c                 |   35 +++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> index 9a81416..37c0ed1 100644
> --- a/include/infiniband/driver.h
> +++ b/include/infiniband/driver.h
> @@ -82,6 +82,12 @@ int ibv_cmd_reg_mr(struct ibv_pd *pd, void *addr, size_t length,
>  		   size_t cmd_size,
>  		   struct ibv_reg_mr_resp *resp, size_t resp_size);
>  int ibv_cmd_dereg_mr(struct ibv_mr *mr);
> +int ibv_cmd_reg_io_mr(struct ibv_pd *pd, void *addr, size_t length,
> +		      uint64_t hca_va, int access,
> +		      struct ibv_mr *mr, struct ibv_reg_io_mr *cmd,
> +		      size_t cmd_size,
> +		      struct ibv_reg_io_mr_resp *resp, size_t resp_size);
> +int ibv_cmd_dereg_io_mr(struct ibv_mr *mr);
>  int ibv_cmd_create_cq(struct ibv_context *context, int cqe,
>  		      struct ibv_comp_channel *channel,
>  		      int comp_vector, struct ibv_cq *cq,
> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
> index 0f1cb2e..a0d969a 100644
> --- a/include/infiniband/verbs.h
> +++ b/include/infiniband/verbs.h
> @@ -640,6 +640,9 @@ struct ibv_context_ops {
>  					    size_t length,
>  					    int access);
>  	int			(*dereg_mr)(struct ibv_mr *mr);
> +        struct ibv_mr *         (*reg_io_mr)(struct ibv_pd *pd, void *addr, size_t length,
> +					     int access);
> +        int                     (*dereg_io_mr)(struct ibv_mr *mr);
>  	struct ibv_mw *		(*alloc_mw)(struct ibv_pd *pd, enum ibv_mw_type type);
>  	int			(*bind_mw)(struct ibv_qp *qp, struct ibv_mw *mw,
>  					   struct ibv_mw_bind *mw_bind);

Doesn't adding these in the middle of the struct break the
libibverbs to libxxxverbs.so binary interface?
Shouldn't they be added at the end of the struct?
I'm not sure how the versioning works between libibverbs and
device plugins. Don't we need to protect against libibverbs
being upgraded but the libxxxverbs.so being older?

> @@ -801,6 +804,17 @@ struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd, void *addr,
>  int ibv_dereg_mr(struct ibv_mr *mr);
>  
>  /**
> + * ibv_reg_io_mr - Register a physical memory region
> + */
> +struct ibv_mr *ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
> +                             size_t length, int access);
> +
> +/**
> + * ibv_dereg_io_mr - Deregister a physical memory region
> + */
> +int ibv_dereg_io_mr(struct ibv_mr *mr);
> +
> +/**
>   * ibv_create_comp_channel - Create a completion event channel
>   */
>  struct ibv_comp_channel *ibv_create_comp_channel(struct ibv_context *context);
> diff --git a/src/verbs.c b/src/verbs.c
> index ba3c0a4..7d215c1 100644
> --- a/src/verbs.c
> +++ b/src/verbs.c
> @@ -189,6 +189,41 @@ int __ibv_dereg_mr(struct ibv_mr *mr)
>  }
>  default_symver(__ibv_dereg_mr, ibv_dereg_mr);
>  
> +struct ibv_mr *__ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
> +                               size_t length, int access)
> +{
> +        struct ibv_mr *mr;
> +
> +        if (ibv_dontfork_range(addr, length))
> +                return NULL;
> +
> +        mr = pd->context->ops.reg_io_mr(pd, addr, length, access);

Won't reg_io_mr pointer be NULL for other HCAs?
What happens if the device doesn't yet implement this function?

> +        if (mr) {
> +                mr->context = pd->context;
> +                mr->pd      = pd;
> +                mr->addr    = addr;
> +                mr->length  = length;
> +        } else
> +                ibv_dofork_range(addr, length);
> +
> +        return mr;
> +}
> +default_symver(__ibv_reg_io_mr, ibv_reg_io_mr);
> +
> +int __ibv_dereg_io_mr(struct ibv_mr *mr)
> +{
> +        int ret;
> +        void *addr      = mr->addr;
> +        size_t length   = mr->length;
> +
> +        ret = mr->context->ops.dereg_io_mr(mr);
> +        if (!ret)
> +                ibv_dofork_range(addr, length);
> +
> +        return ret;
> +}
> +default_symver(__ibv_dereg_io_mr, ibv_dereg_io_mr);
> +
>  static struct ibv_comp_channel *ibv_create_comp_channel_v2(struct ibv_context *context)
>  {
>  	struct ibv_abi_compat_v2 *t = context->abi_compat;
> 
> --
> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs
       [not found]         ` <1280434056.6820.136.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
@ 2010-07-29 20:19           ` Jason Gunthorpe
  2010-07-29 20:32           ` Tom Tucker
  1 sibling, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2010-07-29 20:19 UTC (permalink / raw)
  To: Ralph Campbell
  Cc: Tom Tucker, rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	brandt-4OHPYypu0djtX7QSmKvirg@public.gmane.org,
	swise-/Yg/VP3ZvrM@public.gmane.org

On Thu, Jul 29, 2010 at 01:07:36PM -0700, Ralph Campbell wrote:

> > @@ -640,6 +640,9 @@ struct ibv_context_ops {
> >  					    size_t length,
> >  					    int access);
> >  	int			(*dereg_mr)(struct ibv_mr *mr);
> > +        struct ibv_mr *         (*reg_io_mr)(struct ibv_pd *pd, void *addr, size_t length,
> > +					     int access);
> > +        int                     (*dereg_io_mr)(struct ibv_mr *mr);
> >  	struct ibv_mw *		(*alloc_mw)(struct ibv_pd *pd, enum ibv_mw_type type);
> >  	int			(*bind_mw)(struct ibv_qp *qp, struct ibv_mw *mw,
> >  					   struct ibv_mw_bind *mw_bind);
> 
> Doesn't adding these in the middle of the struct break the
> libibverbs to libxxxverbs.so binary interface?
> Shouldn't they be added at the end of the struct?
> I'm not sure how the versioning works between libibverbs and
> device plugins. Don't we need to protect against libibverbs
> being upgraded but the libxxxverbs.so being older?

Adding them at all, anywhere, breaks the ABI, and needs a soname bump
of libibverbs. Last time this was done a versioned symbol approach was
used to try and keep handling a 1.0 ibv_context structure, but that
only works some times and breaks in corner cases :(

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs
       [not found]         ` <1280434056.6820.136.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-07-29 20:19           ` Jason Gunthorpe
@ 2010-07-29 20:32           ` Tom Tucker
  1 sibling, 0 replies; 7+ messages in thread
From: Tom Tucker @ 2010-07-29 20:32 UTC (permalink / raw)
  To: Ralph Campbell
  Cc: Tom Tucker, rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	brandt-4OHPYypu0djtX7QSmKvirg@public.gmane.org,
	swise-/Yg/VP3ZvrM@public.gmane.org

On 7/29/10 3:07 PM, Ralph Campbell wrote:
> How does an application know when to call ibv_reg_io_mr()
> instead of ibv_reg_mr()? It isn't going to know that some
> address returned by mmap() is going to have the VM_PFNMAP
> flag set.
>    
Please see my response to Jason.
> How does an application know that the HCA supports
> ibv_reg_io_mr() or not? (see below)
> I think returning ENOTSUP or something would be good.
>
>    
There are bits in the devcaps that indicate if these verbs are 
supported. It should however return -ENOTSUPP if they are called without 
support. I copied ibv_reg_mr's which is inappropriate in this regard.
> On Thu, 2010-07-29 at 09:32 -0700, Tom Tucker wrote:
>    
>> From: Tom Tucker<tom-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
>>
>> Add the ibv_reg_io_mr and ibv_dereg_io_mr verbs.
>>
>> Signed-off-by: Tom Tucker<tom-/Yg/VP3ZvrM@public.gmane.org>
>> ---
>>
>>   include/infiniband/driver.h |    6 ++++++
>>   include/infiniband/verbs.h  |   14 ++++++++++++++
>>   src/verbs.c                 |   35 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 55 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
>> index 9a81416..37c0ed1 100644
>> --- a/include/infiniband/driver.h
>> +++ b/include/infiniband/driver.h
>> @@ -82,6 +82,12 @@ int ibv_cmd_reg_mr(struct ibv_pd *pd, void *addr, size_t length,
>>   		   size_t cmd_size,
>>   		   struct ibv_reg_mr_resp *resp, size_t resp_size);
>>   int ibv_cmd_dereg_mr(struct ibv_mr *mr);
>> +int ibv_cmd_reg_io_mr(struct ibv_pd *pd, void *addr, size_t length,
>> +		      uint64_t hca_va, int access,
>> +		      struct ibv_mr *mr, struct ibv_reg_io_mr *cmd,
>> +		      size_t cmd_size,
>> +		      struct ibv_reg_io_mr_resp *resp, size_t resp_size);
>> +int ibv_cmd_dereg_io_mr(struct ibv_mr *mr);
>>   int ibv_cmd_create_cq(struct ibv_context *context, int cqe,
>>   		      struct ibv_comp_channel *channel,
>>   		      int comp_vector, struct ibv_cq *cq,
>> diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
>> index 0f1cb2e..a0d969a 100644
>> --- a/include/infiniband/verbs.h
>> +++ b/include/infiniband/verbs.h
>> @@ -640,6 +640,9 @@ struct ibv_context_ops {
>>   					    size_t length,
>>   					    int access);
>>   	int			(*dereg_mr)(struct ibv_mr *mr);
>> +        struct ibv_mr *         (*reg_io_mr)(struct ibv_pd *pd, void *addr, size_t length,
>> +					     int access);
>> +        int                     (*dereg_io_mr)(struct ibv_mr *mr);
>>   	struct ibv_mw *		(*alloc_mw)(struct ibv_pd *pd, enum ibv_mw_type type);
>>   	int			(*bind_mw)(struct ibv_qp *qp, struct ibv_mw *mw,
>>   					   struct ibv_mw_bind *mw_bind);
>>      
> Doesn't adding these in the middle of the struct break the
> libibverbs to libxxxverbs.so binary interface?
> Shouldn't they be added at the end of the struct?
> I'm not sure how the versioning works between libibverbs and
> device plugins. Don't we need to protect against libibverbs
> being upgraded but the libxxxverbs.so being older?
>
>    
I would think it's broken regardless.

>> @@ -801,6 +804,17 @@ struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd, void *addr,
>>   int ibv_dereg_mr(struct ibv_mr *mr);
>>
>>   /**
>> + * ibv_reg_io_mr - Register a physical memory region
>> + */
>> +struct ibv_mr *ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
>> +                             size_t length, int access);
>> +
>> +/**
>> + * ibv_dereg_io_mr - Deregister a physical memory region
>> + */
>> +int ibv_dereg_io_mr(struct ibv_mr *mr);
>> +
>> +/**
>>    * ibv_create_comp_channel - Create a completion event channel
>>    */
>>   struct ibv_comp_channel *ibv_create_comp_channel(struct ibv_context *context);
>> diff --git a/src/verbs.c b/src/verbs.c
>> index ba3c0a4..7d215c1 100644
>> --- a/src/verbs.c
>> +++ b/src/verbs.c
>> @@ -189,6 +189,41 @@ int __ibv_dereg_mr(struct ibv_mr *mr)
>>   }
>>   default_symver(__ibv_dereg_mr, ibv_dereg_mr);
>>
>> +struct ibv_mr *__ibv_reg_io_mr(struct ibv_pd *pd, void *addr,
>> +                               size_t length, int access)
>> +{
>> +        struct ibv_mr *mr;
>> +
>> +        if (ibv_dontfork_range(addr, length))
>> +                return NULL;
>> +
>> +        mr = pd->context->ops.reg_io_mr(pd, addr, length, access);
>>      
> Won't reg_io_mr pointer be NULL for other HCAs?
> What happens if the device doesn't yet implement this function?
>
>    

Without a check, SEGV. See above.
>> +        if (mr) {
>> +                mr->context = pd->context;
>> +                mr->pd      = pd;
>> +                mr->addr    = addr;
>> +                mr->length  = length;
>> +        } else
>> +                ibv_dofork_range(addr, length);
>> +
>> +        return mr;
>> +}
>> +default_symver(__ibv_reg_io_mr, ibv_reg_io_mr);
>> +
>> +int __ibv_dereg_io_mr(struct ibv_mr *mr)
>> +{
>> +        int ret;
>> +        void *addr      = mr->addr;
>> +        size_t length   = mr->length;
>> +
>> +        ret = mr->context->ops.dereg_io_mr(mr);
>> +        if (!ret)
>> +                ibv_dofork_range(addr, length);
>> +
>> +        return ret;
>> +}
>> +default_symver(__ibv_dereg_io_mr, ibv_dereg_io_mr);
>> +
>>   static struct ibv_comp_channel *ibv_create_comp_channel_v2(struct ibv_context *context)
>>   {
>>   	struct ibv_abi_compat_v2 *t = context->abi_compat;
>>
>> --
>> 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
>    

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2010-07-29 20:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-29 16:31 [RFC PATCH 0/3] libibverbs: Add I/O memory registration verbs Tom Tucker
     [not found] ` <20100729163030.14901.87547.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
2010-07-29 16:31   ` [RFC PATCH 1/3] libibverbs: Fix libtool configure warning Tom Tucker
2010-07-29 16:31   ` [RFC PATCH 2/3] libibverbs: Add reg/unreg I/O memory commands to kern ABI Tom Tucker
2010-07-29 16:32   ` [RFC PATCH 3/3] libibverbs: Add reg/unreg I/O memory verbs Tom Tucker
     [not found]     ` <20100729163202.14901.44211.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
2010-07-29 20:07       ` Ralph Campbell
     [not found]         ` <1280434056.6820.136.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
2010-07-29 20:19           ` Jason Gunthorpe
2010-07-29 20:32           ` Tom Tucker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).