[PATCH libibverbs 0/3] SIF related libibverbs patches

[PATCH libibverbs 0/3] SIF related libibverbs patches

[Knut Omang]

This is the initial patch series for the work of upstreaming the driver and
user level library for SIF, the new Oracle HCA. My philosophy here has been to
make the initial patches as little intrusive as possible, and rather consolidate
later. For instance patch 1 could have just changed the signature of ibv_cmd_create_ah but that
would have required a simultaneous change of all vendor libraries, which is
much easier to accomplish if they all end up in the same repository...

These patches are needed to support SIF - Oracle's new Infiniband HCA stack
from user mode. A corresponding patch set for the kernel will follow shortly.

I have been back and forth with whether or not an ABI version update is required/desired
for the (bugfix) patch 2 in this series, which will be followed by a corresponding patch
at the kernel level. This should ideally have been a backward compatible change since it does not
really logically change the APIs, but because the kernel checks for exact match of
sizes of the cmd structures instead of just accepting the expected size even if the
provided size is larger, both sides need to change simultaneously in this case to avoid
breaking (ibv_reg_mr). In fact, looking at the last ABI change it seems running with version
5 of the ABI on the kernel side is supported, but it will still fail due to the same reason.

No changes are needed for the vendor libraries, except that they need to be rebuilt against the
changed kern-abi.h file due to the same size check in the kernel.

We need this patch because we have a vendor specific extension to the ibv_reg_mr response,
which breaks if alignment criterias are violated.

This patch 2 and the corresponding kernel patch does not update the ABI version,
let me know what you think, if an update is desired, and what MIN and MAX versions
we would want to set.

Patch set:

Knut Omang (3):
  Add new call ibv_cmd_create_ah_ex which supports extra parameters
  Add padding to get proper end alignment of ibv_reg_mr_resp
  Provide remote XRC SRQ number in kernel post_send.

 include/infiniband/driver.h   |  4 ++++
 include/infiniband/kern-abi.h |  2 ++
 src/cmd.c                     | 55 ++++++++++++++++++++++++++-----------------
 src/libibverbs.map            |  1 +
 4 files changed, 40 insertions(+), 22 deletions(-)

--
2.5.5
--
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

The original call ibv_cmd_create_ah does not allow vendor specific request or
response parameters to be defined and passed through to kernel space.

To keep backward compatibility, introduce a new extended call which accepts
cmd and resp parameters similar to other parts of the API.
Reimplement the old call to just use the new extended call.

Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 include/infiniband/driver.h |  4 ++++
 src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
 src/libibverbs.map          |  1 +
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
index 65fa44f..c46c55f 100644
--- a/include/infiniband/driver.h
+++ b/include/infiniband/driver.h
@@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
 			  struct ibv_recv_wr **bad_wr);
 int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
 		      struct ibv_ah_attr *attr);
+int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
+			struct ibv_ah_attr *attr,
+			struct ibv_create_ah *cmd, size_t cmd_size,
+			struct ibv_create_ah_resp *resp, size_t resp_size);
 int ibv_cmd_destroy_ah(struct ibv_ah *ah);
 int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
 int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
diff --git a/src/cmd.c b/src/cmd.c
index cb9e34c..12d8640 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
 	return ret;
 }
 
-int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
-		      struct ibv_ah_attr *attr)
+int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
+			 struct ibv_create_ah *cmd, size_t cmd_size,
+			 struct ibv_create_ah_resp *resp, size_t resp_size)
 {
-	struct ibv_create_ah      cmd;
-	struct ibv_create_ah_resp resp;
-
-	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
-	cmd.user_handle            = (uintptr_t) ah;
-	cmd.pd_handle              = pd->handle;
-	cmd.attr.dlid              = attr->dlid;
-	cmd.attr.sl                = attr->sl;
-	cmd.attr.src_path_bits     = attr->src_path_bits;
-	cmd.attr.static_rate       = attr->static_rate;
-	cmd.attr.is_global         = attr->is_global;
-	cmd.attr.port_num          = attr->port_num;
-	cmd.attr.grh.flow_label    = attr->grh.flow_label;
-	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
-	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
-	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
-	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
+	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
+	cmd->user_handle            = (uintptr_t) ah;
+	cmd->pd_handle              = pd->handle;
+	cmd->attr.dlid              = attr->dlid;
+	cmd->attr.sl                = attr->sl;
+	cmd->attr.src_path_bits     = attr->src_path_bits;
+	cmd->attr.static_rate       = attr->static_rate;
+	cmd->attr.is_global         = attr->is_global;
+	cmd->attr.port_num          = attr->port_num;
+	cmd->attr.grh.flow_label    = attr->grh.flow_label;
+	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
+	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
+	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
+	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
 
-	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
+	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
 		return errno;
 
-	(void) VALGRIND_MAKE_MEM_DEFINED(&resp, sizeof resp);
+	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
 
-	ah->handle  = resp.handle;
+	ah->handle  = resp->handle;
 	ah->context = pd->context;
 
 	return 0;
 }
 
+int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
+		      struct ibv_ah_attr *attr)
+{
+	struct ibv_create_ah      cmd;
+	struct ibv_create_ah_resp resp; 
+	return ibv_cmd_create_ah_ex(pd, ah, attr, &cmd, sizeof(cmd), &resp, sizeof(resp));
+}
+
 int ibv_cmd_destroy_ah(struct ibv_ah *ah)
 {
 	struct ibv_destroy_ah cmd;
diff --git a/src/libibverbs.map b/src/libibverbs.map
index 5134bd9..483df72 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -64,6 +64,7 @@ IBVERBS_1.0 {
 		ibv_cmd_post_recv;
 		ibv_cmd_post_srq_recv;
 		ibv_cmd_create_ah;
+		ibv_cmd_create_ah_ex;
 		ibv_cmd_destroy_ah;
 		ibv_cmd_attach_mcast;
 		ibv_cmd_detach_mcast;
-- 
2.5.5

--
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 libibverbs 2/3] Add padding to get proper end alignment of ibv_reg_mr_resp

[Knut Omang]

The user/kernel level API requires all parameter blocks to be
64 bit end aligned.

Also clean up some valgrind/memory initialization issues.

Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 include/infiniband/kern-abi.h | 1 +
 src/cmd.c                     | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
index f70fa44..8bdeef5 100644
--- a/include/infiniband/kern-abi.h
+++ b/include/infiniband/kern-abi.h
@@ -367,6 +367,7 @@ struct ibv_reg_mr_resp {
 	__u32 mr_handle;
 	__u32 lkey;
 	__u32 rkey;
+	__u32 reserved;
 };
 
 struct ibv_rereg_mr {
diff --git a/src/cmd.c b/src/cmd.c
index 12d8640..a418ee1 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att
 	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
 	cmd->user_handle            = (uintptr_t) ah;
 	cmd->pd_handle              = pd->handle;
+	cmd->reserved               = 0;
 	cmd->attr.dlid              = attr->dlid;
 	cmd->attr.sl                = attr->sl;
 	cmd->attr.src_path_bits     = attr->src_path_bits;
@@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att
 	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
 	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
 	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
+	cmd->attr.grh.reserved      = 0;
+	cmd->attr.reserved          = 0;
 	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
 
 	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
 		return errno;
 
-	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
+	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
 
 	ah->handle  = resp->handle;
 	ah->context = pd->context;
-- 
2.5.5

--
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 libibverbs 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

Also proper end align the ibv_kern_send_wr struct.

Requires a corresponding kernel change.

Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 include/infiniband/kern-abi.h | 1 +
 src/cmd.c                     | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
index 8bdeef5..7b1d310 100644
--- a/include/infiniband/kern-abi.h
+++ b/include/infiniband/kern-abi.h
@@ -800,6 +800,7 @@ struct ibv_kern_send_wr {
 	union {
 		struct {
 			__u32 remote_srqn;
+			__u32 reserved;
 		} xrc;
 	} qp_type;
 };
diff --git a/src/cmd.c b/src/cmd.c
index a418ee1..a4e2f75 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1293,6 +1293,8 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 			tmp->wr.ud.remote_qpn  = i->wr.ud.remote_qpn;
 			tmp->wr.ud.remote_qkey = i->wr.ud.remote_qkey;
 		} else {
+			if (ibqp->qp_type == IBV_QPT_XRC_SEND)
+				tmp->qp_type.xrc.remote_srqn = i->qp_type.xrc.remote_srqn;
 			switch (i->opcode) {
 			case IBV_WR_RDMA_WRITE:
 			case IBV_WR_RDMA_WRITE_WITH_IMM:
-- 
2.5.5

--
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Yishai Hadas]

On 9/1/2016 9:59 AM, Knut Omang wrote:
> The original call ibv_cmd_create_ah does not allow vendor specific request or
> response parameters to be defined and passed through to kernel space.
>
> To keep backward compatibility, introduce a new extended call which accepts
> cmd and resp parameters similar to other parts of the API.
> Reimplement the old call to just use the new extended call.
>
> Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  include/infiniband/driver.h |  4 ++++
>  src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
>  src/libibverbs.map          |  1 +
>  3 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> index 65fa44f..c46c55f 100644
> --- a/include/infiniband/driver.h
> +++ b/include/infiniband/driver.h
> @@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>  			  struct ibv_recv_wr **bad_wr);
>  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
>  		      struct ibv_ah_attr *attr);
> +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
> +			struct ibv_ah_attr *attr,
> +			struct ibv_create_ah *cmd, size_t cmd_size,
> +			struct ibv_create_ah_resp *resp, size_t resp_size);
>  int ibv_cmd_destroy_ah(struct ibv_ah *ah);
>  int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
>  int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> diff --git a/src/cmd.c b/src/cmd.c
> index cb9e34c..12d8640 100644
> --- a/src/cmd.c
> +++ b/src/cmd.c
> @@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>  	return ret;
>  }
>
> -int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> -		      struct ibv_ah_attr *attr)
> +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
> +			 struct ibv_create_ah *cmd, size_t cmd_size,
> +			 struct ibv_create_ah_resp *resp, size_t resp_size)
>  {
> -	struct ibv_create_ah      cmd;
> -	struct ibv_create_ah_resp resp;
> -
> -	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
> -	cmd.user_handle            = (uintptr_t) ah;
> -	cmd.pd_handle              = pd->handle;
> -	cmd.attr.dlid              = attr->dlid;
> -	cmd.attr.sl                = attr->sl;
> -	cmd.attr.src_path_bits     = attr->src_path_bits;
> -	cmd.attr.static_rate       = attr->static_rate;
> -	cmd.attr.is_global         = attr->is_global;
> -	cmd.attr.port_num          = attr->port_num;
> -	cmd.attr.grh.flow_label    = attr->grh.flow_label;
> -	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
> -	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
> -	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
> -	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
> +	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);

You should start with a patch extending the kernel part while supporting 
backwards compatibility, later on when patch applied come with a 
matching user space patch for a review.

In addition, if you supply an extended command you should work with the 
extended macros for issuing a command to fully enable future extensions 
(e.g IBV_INIT_CMD_RESP_EX_V) and with a specific EX command as done in 
other cases (e.g CREATE_QP_EX).

> +	cmd->user_handle            = (uintptr_t) ah;
> +	cmd->pd_handle              = pd->handle;
> +	cmd->attr.dlid              = attr->dlid;
> +	cmd->attr.sl                = attr->sl;
> +	cmd->attr.src_path_bits     = attr->src_path_bits;
> +	cmd->attr.static_rate       = attr->static_rate;
> +	cmd->attr.is_global         = attr->is_global;
> +	cmd->attr.port_num          = attr->port_num;
> +	cmd->attr.grh.flow_label    = attr->grh.flow_label;
> +	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
> +	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
> +	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
> +	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
>
> -	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
> +	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
>  		return errno;
>
> -	(void) VALGRIND_MAKE_MEM_DEFINED(&resp, sizeof resp);
> +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
>
> -	ah->handle  = resp.handle;
> +	ah->handle  = resp->handle;
>  	ah->context = pd->context;
>
>  	return 0;
>  }
>
> +int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> +		      struct ibv_ah_attr *attr)
> +{
> +	struct ibv_create_ah      cmd;
> +	struct ibv_create_ah_resp resp;
> +	return ibv_cmd_create_ah_ex(pd, ah, attr, &cmd, sizeof(cmd), &resp, sizeof(resp));
> +}
> +
>  int ibv_cmd_destroy_ah(struct ibv_ah *ah)
>  {
>  	struct ibv_destroy_ah cmd;
> diff --git a/src/libibverbs.map b/src/libibverbs.map
> index 5134bd9..483df72 100644
> --- a/src/libibverbs.map
> +++ b/src/libibverbs.map
> @@ -64,6 +64,7 @@ IBVERBS_1.0 {
>  		ibv_cmd_post_recv;
>  		ibv_cmd_post_srq_recv;
>  		ibv_cmd_create_ah;
> +		ibv_cmd_create_ah_ex;
>  		ibv_cmd_destroy_ah;
>  		ibv_cmd_attach_mcast;
>  		ibv_cmd_detach_mcast;
>

--
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 libibverbs 2/3] Add padding to get proper end alignment of ibv_reg_mr_resp

[Yishai Hadas]

On 9/1/2016 9:59 AM, Knut Omang wrote:
> The user/kernel level API requires all parameter blocks to be
> 64 bit end aligned.
>
> Also clean up some valgrind/memory initialization issues.
>
> Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  include/infiniband/kern-abi.h | 1 +
>  src/cmd.c                     | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
> index f70fa44..8bdeef5 100644
> --- a/include/infiniband/kern-abi.h
> +++ b/include/infiniband/kern-abi.h
> @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp {
>  	__u32 mr_handle;
>  	__u32 lkey;
>  	__u32 rkey;
> +	__u32 reserved;
>  };
>
>  struct ibv_rereg_mr {
> diff --git a/src/cmd.c b/src/cmd.c
> index 12d8640..a418ee1 100644
> --- a/src/cmd.c
> +++ b/src/cmd.c
> @@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att
>  	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
>  	cmd->user_handle            = (uintptr_t) ah;
>  	cmd->pd_handle              = pd->handle;
> +	cmd->reserved               = 0;
>  	cmd->attr.dlid              = attr->dlid;
>  	cmd->attr.sl                = attr->sl;
>  	cmd->attr.src_path_bits     = attr->src_path_bits;
> @@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att
>  	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
>  	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
>  	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
> +	cmd->attr.grh.reserved      = 0;
> +	cmd->attr.reserved          = 0;

How those changes relate to this patch's subject ? seems that those and 
below relate to previous ibv_cmd_create_ah_ex patch.

>  	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
>
>  	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
>  		return errno;
>
> -	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
>
>  	ah->handle  = resp->handle;
>  	ah->context = pd->context;
>

--
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 libibverbs 3/3] Provide remote XRC SRQ number in kernel post_send.

[Yishai Hadas]

On 9/1/2016 9:59 AM, Knut Omang wrote:
> Also proper end align the ibv_kern_send_wr struct.
>
> Requires a corresponding kernel change.

Again, usually starting from a kernel patch then coming with a matching 
user one.

> Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  include/infiniband/kern-abi.h | 1 +
>  src/cmd.c                     | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
> index 8bdeef5..7b1d310 100644
> --- a/include/infiniband/kern-abi.h
> +++ b/include/infiniband/kern-abi.h
> @@ -800,6 +800,7 @@ struct ibv_kern_send_wr {
>  	union {
>  		struct {
>  			__u32 remote_srqn;
> +			__u32 reserved;
>  		} xrc;
>  	} qp_type;
>  };
> diff --git a/src/cmd.c b/src/cmd.c
> index a418ee1..a4e2f75 100644
> --- a/src/cmd.c
> +++ b/src/cmd.c
> @@ -1293,6 +1293,8 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
>  			tmp->wr.ud.remote_qpn  = i->wr.ud.remote_qpn;
>  			tmp->wr.ud.remote_qkey = i->wr.ud.remote_qkey;
>  		} else {
> +			if (ibqp->qp_type == IBV_QPT_XRC_SEND)
> +				tmp->qp_type.xrc.remote_srqn = i->qp_type.xrc.remote_srqn;
>  			switch (i->opcode) {
>  			case IBV_WR_RDMA_WRITE:
>  			case IBV_WR_RDMA_WRITE_WITH_IMM:
>

--
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 libibverbs 2/3] Add padding to get proper end alignment of ibv_reg_mr_resp

[Knut Omang]

On Thu, 2016-09-01 at 11:56 +0300, Yishai Hadas wrote:
> On 9/1/2016 9:59 AM, Knut Omang wrote:
> > 
> > The user/kernel level API requires all parameter blocks to be
> > 64 bit end aligned.
> > 
> > Also clean up some valgrind/memory initialization issues.
> > 
> > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  include/infiniband/kern-abi.h | 1 +
> >  src/cmd.c                     | 5 ++++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/infiniband/kern-abi.h
> > b/include/infiniband/kern-abi.h
> > index f70fa44..8bdeef5 100644
> > --- a/include/infiniband/kern-abi.h
> > +++ b/include/infiniband/kern-abi.h
> > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp {
> >  	__u32 mr_handle;
> >  	__u32 lkey;
> >  	__u32 rkey;
> > +	__u32 reserved;
> >  };
> > 
> >  struct ibv_rereg_mr {
> > diff --git a/src/cmd.c b/src/cmd.c
> > index 12d8640..a418ee1 100644
> > --- a/src/cmd.c
> > +++ b/src/cmd.c
> > @@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd,
> > struct ibv_ah *ah, struct ibv_ah_att
> >  	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp,
> > resp_size);
> >  	cmd->user_handle            = (uintptr_t) ah;
> >  	cmd->pd_handle              = pd->handle;
> > +	cmd->reserved               = 0;
> >  	cmd->attr.dlid              = attr->dlid;
> >  	cmd->attr.sl                = attr->sl;
> >  	cmd->attr.src_path_bits     = attr->src_path_bits;
> > @@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd,
> > struct ibv_ah *ah, struct ibv_ah_att
> >  	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
> >  	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
> >  	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
> > +	cmd->attr.grh.reserved      = 0;
> > +	cmd->attr.reserved          = 0;
> How those changes relate to this patch's subject ? seems that those
> and 
> below relate to previous ibv_cmd_create_ah_ex patch.

These are fixing valgrind complaints in the original code, but because
that code got rewritten by the previous patch, it might look that way.

The initial idea was that this whole patch fixes trivial bugs/issues in
the current libibverbs, while the other two provides new functionality.
I can split this out as a separate patch for the valgrind part, if
necessary.

Knut

> 
> > 
> >  	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
> > 
> >  	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
> >  		return errno;
> > 
> > -	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> > +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
> > 
> >  	ah->handle  = resp->handle;
> >  	ah->context = pd->context;
> > 
--
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Thu, 2016-09-01 at 11:34 +0300, Yishai Hadas wrote:
> On 9/1/2016 9:59 AM, Knut Omang wrote:
> > 
> > The original call ibv_cmd_create_ah does not allow vendor specific request or
> > response parameters to be defined and passed through to kernel space.
> > 
> > To keep backward compatibility, introduce a new extended call which accepts
> > cmd and resp parameters similar to other parts of the API.
> > Reimplement the old call to just use the new extended call.
> > 
> > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  include/infiniband/driver.h |  4 ++++
> >  src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
> >  src/libibverbs.map          |  1 +
> >  3 files changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > index 65fa44f..c46c55f 100644
> > --- a/include/infiniband/driver.h
> > +++ b/include/infiniband/driver.h
> > @@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> >  			  struct ibv_recv_wr **bad_wr);
> >  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> >  		      struct ibv_ah_attr *attr);
> > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
> > +			struct ibv_ah_attr *attr,
> > +			struct ibv_create_ah *cmd, size_t cmd_size,
> > +			struct ibv_create_ah_resp *resp, size_t resp_size);
> >  int ibv_cmd_destroy_ah(struct ibv_ah *ah);
> >  int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> >  int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> > diff --git a/src/cmd.c b/src/cmd.c
> > index cb9e34c..12d8640 100644
> > --- a/src/cmd.c
> > +++ b/src/cmd.c
> > @@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> >  	return ret;
> >  }
> > 
> > -int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > -		      struct ibv_ah_attr *attr)
> > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
> > +			 struct ibv_create_ah *cmd, size_t cmd_size,
> > +			 struct ibv_create_ah_resp *resp, size_t resp_size)
> >  {
> > -	struct ibv_create_ah      cmd;
> > -	struct ibv_create_ah_resp resp;
> > -
> > -	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
> > -	cmd.user_handle            = (uintptr_t) ah;
> > -	cmd.pd_handle              = pd->handle;
> > -	cmd.attr.dlid              = attr->dlid;
> > -	cmd.attr.sl                = attr->sl;
> > -	cmd.attr.src_path_bits     = attr->src_path_bits;
> > -	cmd.attr.static_rate       = attr->static_rate;
> > -	cmd.attr.is_global         = attr->is_global;
> > -	cmd.attr.port_num          = attr->port_num;
> > -	cmd.attr.grh.flow_label    = attr->grh.flow_label;
> > -	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
> > -	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
> > -	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
> > -	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
> > +	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
> You should start with a patch extending the kernel part while supporting 
> backwards compatibility, later on when patch applied come with a 
> matching user space patch for a review.

Coming very soon, I realize that the order should have been the opposite.

> In addition, if you supply an extended command you should work with the 
> extended macros for issuing a command to fully enable future extensions 
> (e.g IBV_INIT_CMD_RESP_EX_V) and with a specific EX command as done in 
> other cases (e.g CREATE_QP_EX).

Hmm - I see - I wrote this patch long before that EX_V logic got added 
- I can revisit.

Thanks for the quick reviews,

Knut

> > +	cmd->user_handle            = (uintptr_t) ah;
> > +	cmd->pd_handle              = pd->handle;
> > +	cmd->attr.dlid              = attr->dlid;
> > +	cmd->attr.sl                = attr->sl;
> > +	cmd->attr.src_path_bits     = attr->src_path_bits;
> > +	cmd->attr.static_rate       = attr->static_rate;
> > +	cmd->attr.is_global         = attr->is_global;
> > +	cmd->attr.port_num          = attr->port_num;
> > +	cmd->attr.grh.flow_label    = attr->grh.flow_label;
> > +	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
> > +	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
> > +	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
> > +	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
> > 
> > -	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
> > +	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
> >  		return errno;
> > 
> > -	(void) VALGRIND_MAKE_MEM_DEFINED(&resp, sizeof resp);
> > +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> > 
> > -	ah->handle  = resp.handle;
> > +	ah->handle  = resp->handle;
> >  	ah->context = pd->context;
> > 
> >  	return 0;
> >  }
> > 
> > +int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > +		      struct ibv_ah_attr *attr)
> > +{
> > +	struct ibv_create_ah      cmd;
> > +	struct ibv_create_ah_resp resp;
> > +	return ibv_cmd_create_ah_ex(pd, ah, attr, &cmd, sizeof(cmd), &resp, sizeof(resp));
> > +}
> > +
> >  int ibv_cmd_destroy_ah(struct ibv_ah *ah)
> >  {
> >  	struct ibv_destroy_ah cmd;
> > diff --git a/src/libibverbs.map b/src/libibverbs.map
> > index 5134bd9..483df72 100644
> > --- a/src/libibverbs.map
> > +++ b/src/libibverbs.map
> > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> >  		ibv_cmd_post_recv;
> >  		ibv_cmd_post_srq_recv;
> >  		ibv_cmd_create_ah;
> > +		ibv_cmd_create_ah_ex;
> >  		ibv_cmd_destroy_ah;
> >  		ibv_cmd_attach_mcast;
> >  		ibv_cmd_detach_mcast;
> > 
--
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 libibverbs 2/3] Add padding to get proper end alignment of ibv_reg_mr_resp

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 08:59:52AM +0200, Knut Omang wrote:
> The user/kernel level API requires all parameter blocks to be
> 64 bit end aligned.
> 
> Also clean up some valgrind/memory initialization issues.
> 
> Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>  include/infiniband/kern-abi.h | 1 +
>  src/cmd.c                     | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
> index f70fa44..8bdeef5 100644
> +++ b/include/infiniband/kern-abi.h
> @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp {
>  	__u32 mr_handle;
>  	__u32 lkey;
>  	__u32 rkey;
> +	__u32 reserved;
>  };

This structure is a copy of include/uapi/rdma/ib_user_verbs.h, so you
need to start with a kernel patch proposing this change.

We will eventually get rid of kern-abi.h and use ib_user_verbs.h at some
point.

Maybe elaborate on why this is OK and doesn't break anything in that
patch..

> -	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);

Why the (void)?

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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> +++ b/src/libibverbs.map
> @@ -64,6 +64,7 @@ IBVERBS_1.0 {
>  		ibv_cmd_post_recv;
>  		ibv_cmd_post_srq_recv;
>  		ibv_cmd_create_ah;
> +		ibv_cmd_create_ah_ex;

I should also point out that this is not the proper way to use symbol
versions. Typically one would tag new symbols with the version number
of the release that introduces them, and not just keep re-using 1.0

I know we haven't been doing that, but perhaps we should start.

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 libibverbs 2/3] Add padding to get proper end alignment of ibv_reg_mr_resp

[Knut Omang]

On Thu, 2016-09-01 at 10:42 -0600, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2016 at 08:59:52AM +0200, Knut Omang wrote:
> > The user/kernel level API requires all parameter blocks to be
> > 64 bit end aligned.
> > 
> > Also clean up some valgrind/memory initialization issues.
> > 
> > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >  include/infiniband/kern-abi.h | 1 +
> >  src/cmd.c                     | 5 ++++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
> > index f70fa44..8bdeef5 100644
> > +++ b/include/infiniband/kern-abi.h
> > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp {
> >  	__u32 mr_handle;
> >  	__u32 lkey;
> >  	__u32 rkey;
> > +	__u32 reserved;
> >  };
> 
> This structure is a copy of include/uapi/rdma/ib_user_verbs.h, so you
> need to start with a kernel patch proposing this change.

Yes, I am aware of that, sorry - I intended to post both sets in sequence but ran into 
a few recently introduced conflicts in the kernel set, so still working on that 
(just testing remains)

> We will eventually get rid of kern-abi.h and use ib_user_verbs.h at some
> point.

Good to get a common file for it and get rid of some copy/paste.

> Maybe elaborate on why this is OK and doesn't break anything in that
> patch..
> 
> > -	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> > +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
> 
> Why the (void)?

I guess it is there to avoid compiler warnings about unused variables in some 
configuration. I just kept it as it was in this patch. I see all instances 
throughout the code has this.

Thanks,
Knut

> 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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Thu, 2016-09-01 at 10:49 -0600, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > +++ b/src/libibverbs.map
> > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> >  		ibv_cmd_post_recv;
> >  		ibv_cmd_post_srq_recv;
> >  		ibv_cmd_create_ah;
> > +		ibv_cmd_create_ah_ex;
> 
> I should also point out that this is not the proper way to use symbol
> versions. Typically one would tag new symbols with the version number
> of the release that introduces them, and not just keep re-using 1.0
> 
> I know we haven't been doing that, but perhaps we should start.

Yes, I noticed the never changed 1.0 and tried just to follow the usage 
pattern ;-)

Note that the best solution if we get a merged library repo would just be to 
eliminate it by folding the arguments into ibv_cmd_create_ah and make it similar to
the other *create* functions.

I added it only to avoid breaking vendor libraries.

Knut

> 
> 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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 07:22:38PM +0200, Knut Omang wrote:

> Note that the best solution if we get a merged library repo would just be to 
> eliminate it by folding the arguments into ibv_cmd_create_ah and make it similar to
> the other *create* functions.

Well, we currently have a promise not to break the ABI facing the
providers.

So, we'd still have to use symbol versions when we change the cmd so
old providers link. Not sure how much win there is there vs _ex.

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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 07:22:38PM +0200, Knut Omang wrote:
> On Thu, 2016-09-01 at 10:49 -0600, Jason Gunthorpe wrote:
> > On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > > +++ b/src/libibverbs.map
> > > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> > >  		ibv_cmd_post_recv;
> > >  		ibv_cmd_post_srq_recv;
> > >  		ibv_cmd_create_ah;
> > > +		ibv_cmd_create_ah_ex;
> > 
> > I should also point out that this is not the proper way to use symbol
> > versions. Typically one would tag new symbols with the version number
> > of the release that introduces them, and not just keep re-using 1.0
> > 
> > I know we haven't been doing that, but perhaps we should start.
> 
> Yes, I noticed the never changed 1.0 and tried just to follow the usage 
> pattern ;-)

Actually, thinking about this more, not doing this properly breaks RPM
since it only summarizes the tag not the actual symbol into the
package data.

So, no this is not OK, and maybe we need to retroactively review
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Thu, 2016-09-01 at 12:05 -0600, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2016 at 07:22:38PM +0200, Knut Omang wrote:
> > On Thu, 2016-09-01 at 10:49 -0600, Jason Gunthorpe wrote:
> > > On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > > > +++ b/src/libibverbs.map
> > > > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> > > >  		ibv_cmd_post_recv;
> > > >  		ibv_cmd_post_srq_recv;
> > > >  		ibv_cmd_create_ah;
> > > > +		ibv_cmd_create_ah_ex;
> > > 
> > > I should also point out that this is not the proper way to use symbol
> > > versions. Typically one would tag new symbols with the version number
> > > of the release that introduces them, and not just keep re-using 1.0
> > > 
> > > I know we haven't been doing that, but perhaps we should start.
> > 
> > Yes, I noticed the never changed 1.0 and tried just to follow the usage 
> > pattern ;-)
> 
> Actually, thinking about this more, not doing this properly breaks RPM
> since it only summarizes the tag not the actual symbol into the
> package data.
> 
> So, no this is not OK, and maybe we need to retroactively review
> things :( :(

For this patch backward binary compatibility is actually preserved - a vendor library 
continues to work even without recompiling. I deliberately avoided making
the ibv_cmd_create_ah function inline to ensure that. Loading libsif would of course
break, but with a symbol error.

But versioning is still an issue to consider with patch 2 in this set, as it 
unfortunately has to change the layout of the data structures between 
kernel and user space to fix the alignment bug.

Patch 3 has the same alignment issue, but is not used by anyone but our
new vendor library.

Knut

> 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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 08:23:40PM +0200, Knut Omang wrote:
> > > > On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > > > > +++ b/src/libibverbs.map
> > > > > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> > > > >  		ibv_cmd_post_recv;
> > > > >  		ibv_cmd_post_srq_recv;
> > > > >  		ibv_cmd_create_ah;
> > > > > +		ibv_cmd_create_ah_ex;

> For this patch backward binary compatibility is actually preserved - a vendor library 
> continues to work even without recompiling. I deliberately avoided making
> the ibv_cmd_create_ah function inline to ensure that. Loading libsif would of course
> break, but with a symbol error.

No, this patch is wrong, you added it to IBVERBS_1.0, that isn't
allowed.

You need to do something like this:

IBVERBS_1.3 {
        global:
	   ibv_cmd_create_ah_ex;

Why? RPM works by having provides like:

 libibverbs
 libibverbs(x86-64)
 libibverbs.so.1()(64bit)
 libibverbs.so.1(IBVERBS_1.0)(64bit)
 libibverbs.so.1(IBVERBS_1.1)(64bit)

And when you create a client RPM package it strips off the function name
and uniques it to produce the Depends.

In other words, once IBVERBS_1.x is shipped in a RPM that stanza in
the map file is set in stone. We can never add more symbols to that
stanza. We must start a new number.

To be concrete, when your new provider links to
ibv_cmd_create_ah_ex@IBVERBS_1.0 rpm will create a dependency
libibverbs.so.1(IBVERBS_1.0).  *However* every RPM provides that, so
the dependency logic in RPM stops working and will let a user install
your provider on a system it is not compatible with. When it is set
properly to ibv_cmd_create_ah_ex@IBVERBS_1.3 then RPM will only allow
the provider RPM to be installed on a system that has the upgraded
libibverbs1.

Unfortunately this has been systematically screwed up by recent
patches (pay attention Matan and Yisahi), so our symbol versioning is
now unusable for RPM based distros.

Debian does this differently and will not be affected. Actual linking
should be fine too, AFAIK it is just RPM that will break.

Doug? Is it too late to fix this for the patches you accepted in 2016?
The answer to that depends entirely on how far the RPMs have got..

The _cmd_ varients are not too bad, we could fix that in rdma-plumbing
in a way that lets us move forward sane sanely, but
ibv_query_device_ex@IBVERBS_1.0 should have been tagged _1.2

The fix would be to introduce a
ibv_query_device_ex@IBVERBS_1.3 as the new default and continue to
provide ibv_query_device_ex@IBVERBS_1.0 for compat against the same
physical symbol. New links would then get functional rpm dependencies.

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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Thu, 2016-09-01 at 20:06 -0600, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2016 at 08:23:40PM +0200, Knut Omang wrote:
> > 
> > > 
> > > > 
> > > > > 
> > > > > On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > > > > > 
> > > > > > +++ b/src/libibverbs.map
> > > > > > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> > > > > >  		ibv_cmd_post_recv;
> > > > > >  		ibv_cmd_post_srq_recv;
> > > > > >  		ibv_cmd_create_ah;
> > > > > > +		ibv_cmd_create_ah_ex;
> > 
> > For this patch backward binary compatibility is actually preserved - a vendor library 
> > continues to work even without recompiling. I deliberately avoided making
> > the ibv_cmd_create_ah function inline to ensure that. Loading libsif would of course
> > break, but with a symbol error.
> No, this patch is wrong, you added it to IBVERBS_1.0, that isn't
> allowed.
> 
> You need to do something like this:
> 
> IBVERBS_1.3 {
>         global:
> 	   ibv_cmd_create_ah_ex;

You are quite right - I'll fix the patch like you indicate, using either 1.2 or 1.3 depending on
the conclusions on the other changes you mention below, Doug, let me know what value you want,

Thanks,
Knut

> Why? RPM works by having provides like:
> 
>  libibverbs
>  libibverbs(x86-64)
>  libibverbs.so.1()(64bit)
>  libibverbs.so.1(IBVERBS_1.0)(64bit)
>  libibverbs.so.1(IBVERBS_1.1)(64bit)
> 
> And when you create a client RPM package it strips off the function name
> and uniques it to produce the Depends.
> 
> In other words, once IBVERBS_1.x is shipped in a RPM that stanza in
> the map file is set in stone. We can never add more symbols to that
> stanza. We must start a new number.
> 
> To be concrete, when your new provider links to
> ibv_cmd_create_ah_ex@IBVERBS_1.0 rpm will create a dependency
> libibverbs.so.1(IBVERBS_1.0).  *However* every RPM provides that, so
> the dependency logic in RPM stops working and will let a user install
> your provider on a system it is not compatible with. When it is set
> properly to ibv_cmd_create_ah_ex@IBVERBS_1.3 then RPM will only allow
> the provider RPM to be installed on a system that has the upgraded
> libibverbs1.
> 
> Unfortunately this has been systematically screwed up by recent
> patches (pay attention Matan and Yisahi), so our symbol versioning is
> now unusable for RPM based distros.
> 
> Debian does this differently and will not be affected. Actual linking
> should be fine too, AFAIK it is just RPM that will break.
> 
> Doug? Is it too late to fix this for the patches you accepted in 2016?
> The answer to that depends entirely on how far the RPMs have got..
> 
> The _cmd_ varients are not too bad, we could fix that in rdma-plumbing
> in a way that lets us move forward sane sanely, but
> ibv_query_device_ex@IBVERBS_1.0 should have been tagged _1.2
> 
> The fix would be to introduce a
> ibv_query_device_ex@IBVERBS_1.3 as the new default and continue to
> provide ibv_query_device_ex@IBVERBS_1.0 for compat against the same
> physical symbol. New links would then get functional rpm dependencies.
> 
> 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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Fri, 2016-09-02 at 09:49 +0200, Knut Omang wrote:
> On Thu, 2016-09-01 at 20:06 -0600, Jason Gunthorpe wrote:
> > On Thu, Sep 01, 2016 at 08:23:40PM +0200, Knut Omang wrote:
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > On Thu, Sep 01, 2016 at 08:59:51AM +0200, Knut Omang wrote:
> > > > > > > 
> > > > > > > +++ b/src/libibverbs.map
> > > > > > > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> > > > > > >  		ibv_cmd_post_recv;
> > > > > > >  		ibv_cmd_post_srq_recv;
> > > > > > >  		ibv_cmd_create_ah;
> > > > > > > +		ibv_cmd_create_ah_ex;
> > > 
> > > For this patch backward binary compatibility is actually preserved - a vendor library 
> > > continues to work even without recompiling. I deliberately avoided making
> > > the ibv_cmd_create_ah function inline to ensure that. Loading libsif would of course
> > > break, but with a symbol error.
> > No, this patch is wrong, you added it to IBVERBS_1.0, that isn't
> > allowed.
> > 
> > You need to do something like this:
> > 
> > IBVERBS_1.3 {
> >         global:
> > 	   ibv_cmd_create_ah_ex;
> 
> You are quite right - I'll fix the patch like you indicate, using either 1.2 or 1.3 depending on
> the conclusions on the other changes you mention below, Doug, let me know what value you want,
> 
> Thanks,
> Knut
> 
> > Why? RPM works by having provides like:
> > 
> >  libibverbs
> >  libibverbs(x86-64)
> >  libibverbs.so.1()(64bit)
> >  libibverbs.so.1(IBVERBS_1.0)(64bit)
> >  libibverbs.so.1(IBVERBS_1.1)(64bit)
> > 
> > And when you create a client RPM package it strips off the function name
> > and uniques it to produce the Depends.
> > 
> > In other words, once IBVERBS_1.x is shipped in a RPM that stanza in
> > the map file is set in stone. We can never add more symbols to that
> > stanza. We must start a new number.
> > 
> > To be concrete, when your new provider links to
> > ibv_cmd_create_ah_ex@IBVERBS_1.0 rpm will create a dependency
> > libibverbs.so.1(IBVERBS_1.0).  *However* every RPM provides that, so
> > the dependency logic in RPM stops working and will let a user install
> > your provider on a system it is not compatible with. When it is set
> > properly to ibv_cmd_create_ah_ex@IBVERBS_1.3 then RPM will only allow
> > the provider RPM to be installed on a system that has the upgraded
> > libibverbs1.
> > 
> > Unfortunately this has been systematically screwed up by recent
> > patches (pay attention Matan and Yisahi), so our symbol versioning is
> > now unusable for RPM based distros.
> > 
> > Debian does this differently and will not be affected. Actual linking
> > should be fine too, AFAIK it is just RPM that will break.
> > 
> > Doug? Is it too late to fix this for the patches you accepted in 2016?
> > The answer to that depends entirely on how far the RPMs have got..
> > 
> > The _cmd_ varients are not too bad, we could fix that in rdma-plumbing
> > in a way that lets us move forward sane sanely, but
> > ibv_query_device_ex@IBVERBS_1.0 should have been tagged _1.2
> > 
> > The fix would be to introduce a
> > ibv_query_device_ex@IBVERBS_1.3 as the new default and continue to
> > provide ibv_query_device_ex@IBVERBS_1.0 for compat against the same
> > physical symbol. New links would then get functional rpm dependencies.

Looking more closely at this whole problem, I think for my patch, 
the right solution is just to avoid adding a new function name and 
instead provide a redefinition of the original symbol with a new version 
using the __asm__(".symver ...) construct already nicely used for v.1.0 
bw compatibility - just following the pattern already established.

That way old provider builds can continue to work seamlessly and binary compatible, 
but when recompiling against the new version, the compiler enforces use of the 
new definitions instead, implicitly enforcing cleanup, which is a good thing..

I think in principle this could have been done for 
the other _ex functions as well, getting rid of the whole 
multiple _ex structs stuff.

Knut


--
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 09:30:40AM +0200, Knut Omang wrote:

> Looking more closely at this whole problem, I think for my patch, 
> the right solution is just to avoid adding a new function name and 
> instead provide a redefinition of the original symbol with a new version 
> using the __asm__(".symver ...) construct already nicely used for v.1.0 
> bw compatibility - just following the pattern already established.

That is possible, but to date we have never done that and have
continued to preserve source compatability for providers.

I agree the providers should be updated, but I don't see this being
practical until after rdma-plumbing is in place..

So I suggest you proceed as you have with the fixed symbol version.

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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Sun, 2016-09-04 at 20:38 -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 09:30:40AM +0200, Knut Omang wrote:
> 
> > Looking more closely at this whole problem, I think for my patch, 
> > the right solution is just to avoid adding a new function name and 
> > instead provide a redefinition of the original symbol with a new version 
> > using the __asm__(".symver ...) construct already nicely used for v.1.0 
> > bw compatibility - just following the pattern already established.
> 
> That is possible, but to date we have never done that and have
> continued to preserve source compatability for providers.
> 
> I agree the providers should be updated, but I don't see this being
> practical until after rdma-plumbing is in place..
> 
> So I suggest you proceed as you have with the fixed symbol version.

Ok, fine - I have implemented and tested both solutions with current libmlx4 + 
Connect-X3 and libsif + SIF using the libibverbs and kernel patch sets in the same box, 
with a small amendment to the size checking in the uverbs ibv_reg_mr path and feel 
I am ready for a v2 then.

Thanks,
Knut

> 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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Thu, 2016-09-01 at 11:34 +0300, Yishai Hadas wrote:
> On 9/1/2016 9:59 AM, Knut Omang wrote:
> > 
> > The original call ibv_cmd_create_ah does not allow vendor specific request or
> > response parameters to be defined and passed through to kernel space.
> > 
> > To keep backward compatibility, introduce a new extended call which accepts
> > cmd and resp parameters similar to other parts of the API.
> > Reimplement the old call to just use the new extended call.
> > 
> > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  include/infiniband/driver.h |  4 ++++
> >  src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
> >  src/libibverbs.map          |  1 +
> >  3 files changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > index 65fa44f..c46c55f 100644
> > --- a/include/infiniband/driver.h
> > +++ b/include/infiniband/driver.h
> > @@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> >  			  struct ibv_recv_wr **bad_wr);
> >  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> >  		      struct ibv_ah_attr *attr);
> > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
> > +			struct ibv_ah_attr *attr,
> > +			struct ibv_create_ah *cmd, size_t cmd_size,
> > +			struct ibv_create_ah_resp *resp, size_t resp_size);
> >  int ibv_cmd_destroy_ah(struct ibv_ah *ah);
> >  int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> >  int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> > diff --git a/src/cmd.c b/src/cmd.c
> > index cb9e34c..12d8640 100644
> > --- a/src/cmd.c
> > +++ b/src/cmd.c
> > @@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> >  	return ret;
> >  }
> > 
> > -int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > -		      struct ibv_ah_attr *attr)
> > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
> > +			 struct ibv_create_ah *cmd, size_t cmd_size,
> > +			 struct ibv_create_ah_resp *resp, size_t resp_size)
> >  {
> > -	struct ibv_create_ah      cmd;
> > -	struct ibv_create_ah_resp resp;
> > -
> > -	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
> > -	cmd.user_handle            = (uintptr_t) ah;
> > -	cmd.pd_handle              = pd->handle;
> > -	cmd.attr.dlid              = attr->dlid;
> > -	cmd.attr.sl                = attr->sl;
> > -	cmd.attr.src_path_bits     = attr->src_path_bits;
> > -	cmd.attr.static_rate       = attr->static_rate;
> > -	cmd.attr.is_global         = attr->is_global;
> > -	cmd.attr.port_num          = attr->port_num;
> > -	cmd.attr.grh.flow_label    = attr->grh.flow_label;
> > -	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
> > -	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
> > -	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
> > -	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
> > +	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
> You should start with a patch extending the kernel part while supporting 
> backwards compatibility, later on when patch applied come with a 
> matching user space patch for a review.
> 
> In addition, if you supply an extended command you should work with the 
> extended macros for issuing a command to fully enable future extensions 
> (e.g IBV_INIT_CMD_RESP_EX_V) and with a specific EX command as done in 
> other cases (e.g CREATE_QP_EX).

Having looked at the details, I tend to think that what I am adding here is not
an extended command, it is more of a fix for a missing feature of 
the original ibv_cmd_create_ah, and the only reason for introducing a new 
call is to maintain backward comp. Introducing additional complexity 
with the kernel involved for something that is a pure user level side
seems somewhat overkill.

Maybe I can rename it to ibv_cmd_create_ah_v2 (or v3?) to avoid associating it
with the extended command set? 

As Jason and I discussed in another subthread here, we can consolidate this into a 
single command once we get to a common library,
as this is all internal to the rdma user level stack.

Ok?

Knut

> > 
> > +	cmd->user_handle            = (uintptr_t) ah;
> > +	cmd->pd_handle              = pd->handle;
> > +	cmd->attr.dlid              = attr->dlid;
> > +	cmd->attr.sl                = attr->sl;
> > +	cmd->attr.src_path_bits     = attr->src_path_bits;
> > +	cmd->attr.static_rate       = attr->static_rate;
> > +	cmd->attr.is_global         = attr->is_global;
> > +	cmd->attr.port_num          = attr->port_num;
> > +	cmd->attr.grh.flow_label    = attr->grh.flow_label;
> > +	cmd->attr.grh.sgid_index    = attr->grh.sgid_index;
> > +	cmd->attr.grh.hop_limit     = attr->grh.hop_limit;
> > +	cmd->attr.grh.traffic_class = attr->grh.traffic_class;
> > +	memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16);
> > 
> > -	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
> > +	if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size)
> >  		return errno;
> > 
> > -	(void) VALGRIND_MAKE_MEM_DEFINED(&resp, sizeof resp);
> > +	(void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
> > 
> > -	ah->handle  = resp.handle;
> > +	ah->handle  = resp->handle;
> >  	ah->context = pd->context;
> > 
> >  	return 0;
> >  }
> > 
> > +int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > +		      struct ibv_ah_attr *attr)
> > +{
> > +	struct ibv_create_ah      cmd;
> > +	struct ibv_create_ah_resp resp;
> > +	return ibv_cmd_create_ah_ex(pd, ah, attr, &cmd, sizeof(cmd), &resp, sizeof(resp));
> > +}
> > +
> >  int ibv_cmd_destroy_ah(struct ibv_ah *ah)
> >  {
> >  	struct ibv_destroy_ah cmd;
> > diff --git a/src/libibverbs.map b/src/libibverbs.map
> > index 5134bd9..483df72 100644
> > --- a/src/libibverbs.map
> > +++ b/src/libibverbs.map
> > @@ -64,6 +64,7 @@ IBVERBS_1.0 {
> >  		ibv_cmd_post_recv;
> >  		ibv_cmd_post_srq_recv;
> >  		ibv_cmd_create_ah;
> > +		ibv_cmd_create_ah_ex;
> >  		ibv_cmd_destroy_ah;
> >  		ibv_cmd_attach_mcast;
> >  		ibv_cmd_detach_mcast;
> > 
> --
> 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

Re: [PATCH libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Yishai Hadas]

On 9/5/2016 2:53 PM, Knut Omang wrote:
> On Thu, 2016-09-01 at 11:34 +0300, Yishai Hadas wrote:
>> On 9/1/2016 9:59 AM, Knut Omang wrote:
>>>
>>> The original call ibv_cmd_create_ah does not allow vendor specific request or
>>> response parameters to be defined and passed through to kernel space.
>>>
>>> To keep backward compatibility, introduce a new extended call which accepts
>>> cmd and resp parameters similar to other parts of the API.
>>> Reimplement the old call to just use the new extended call.
>>>
>>> Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  include/infiniband/driver.h |  4 ++++
>>>  src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
>>>  src/libibverbs.map          |  1 +
>>>  3 files changed, 33 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
>>> index 65fa44f..c46c55f 100644
>>> --- a/include/infiniband/driver.h
>>> +++ b/include/infiniband/driver.h
>>> @@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>>>  			  struct ibv_recv_wr **bad_wr);
>>>  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
>>>  		      struct ibv_ah_attr *attr);
>>> +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
>>> +			struct ibv_ah_attr *attr,
>>> +			struct ibv_create_ah *cmd, size_t cmd_size,
>>> +			struct ibv_create_ah_resp *resp, size_t resp_size);
>>>  int ibv_cmd_destroy_ah(struct ibv_ah *ah);
>>>  int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
>>>  int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
>>> diff --git a/src/cmd.c b/src/cmd.c
>>> index cb9e34c..12d8640 100644
>>> --- a/src/cmd.c
>>> +++ b/src/cmd.c
>>> @@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>>>  	return ret;
>>>  }
>>>
>>> -int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
>>> -		      struct ibv_ah_attr *attr)
>>> +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
>>> +			 struct ibv_create_ah *cmd, size_t cmd_size,
>>> +			 struct ibv_create_ah_resp *resp, size_t resp_size)
>>>  {
>>> -	struct ibv_create_ah      cmd;
>>> -	struct ibv_create_ah_resp resp;
>>> -
>>> -	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
>>> -	cmd.user_handle            = (uintptr_t) ah;
>>> -	cmd.pd_handle              = pd->handle;
>>> -	cmd.attr.dlid              = attr->dlid;
>>> -	cmd.attr.sl                = attr->sl;
>>> -	cmd.attr.src_path_bits     = attr->src_path_bits;
>>> -	cmd.attr.static_rate       = attr->static_rate;
>>> -	cmd.attr.is_global         = attr->is_global;
>>> -	cmd.attr.port_num          = attr->port_num;
>>> -	cmd.attr.grh.flow_label    = attr->grh.flow_label;
>>> -	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
>>> -	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
>>> -	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
>>> -	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
>>> +	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
>> You should start with a patch extending the kernel part while supporting
>> backwards compatibility, later on when patch applied come with a
>> matching user space patch for a review.
>>
>> In addition, if you supply an extended command you should work with the
>> extended macros for issuing a command to fully enable future extensions
>> (e.g IBV_INIT_CMD_RESP_EX_V) and with a specific EX command as done in
>> other cases (e.g CREATE_QP_EX).
>
> Having looked at the details, I tend to think that what I am adding here is not
> an extended command, it is more of a fix for a missing feature of
> the original ibv_cmd_create_ah,

You want to extend the user->kernel API to get some extra data while 
maintaining BW compatibility. Usually you need for that an extra command 
which better be extended from day one. Can it be done in the kernel side 
without a new command ? please send pointer to the matching kernel patch.

> Maybe I can rename it to ibv_cmd_create_ah_v2 (or v3?) to avoid associating it
> with the extended command set?

In case you need a new command, let's add it in an extended mode so that 
future extensions will use it instead of having later v3/v4, etc.

--
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 libibverbs 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Knut Omang]

On Mon, 2016-09-05 at 17:05 +0300, Yishai Hadas wrote:
> On 9/5/2016 2:53 PM, Knut Omang wrote:
> > On Thu, 2016-09-01 at 11:34 +0300, Yishai Hadas wrote:
> > > On 9/1/2016 9:59 AM, Knut Omang wrote:
> > > > 
> > > > The original call ibv_cmd_create_ah does not allow vendor specific request or
> > > > response parameters to be defined and passed through to kernel space.
> > > > 
> > > > To keep backward compatibility, introduce a new extended call which accepts
> > > > cmd and resp parameters similar to other parts of the API.
> > > > Reimplement the old call to just use the new extended call.
> > > > 
> > > > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  include/infiniband/driver.h |  4 ++++
> > > >  src/cmd.c                   | 50 +++++++++++++++++++++++++--------------------
> > > >  src/libibverbs.map          |  1 +
> > > >  3 files changed, 33 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > > > index 65fa44f..c46c55f 100644
> > > > --- a/include/infiniband/driver.h
> > > > +++ b/include/infiniband/driver.h
> > > > @@ -229,6 +229,10 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> > > >  			  struct ibv_recv_wr **bad_wr);
> > > >  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > > >  		      struct ibv_ah_attr *attr);
> > > > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah,
> > > > +			struct ibv_ah_attr *attr,
> > > > +			struct ibv_create_ah *cmd, size_t cmd_size,
> > > > +			struct ibv_create_ah_resp *resp, size_t resp_size);
> > > >  int ibv_cmd_destroy_ah(struct ibv_ah *ah);
> > > >  int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> > > >  int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
> > > > diff --git a/src/cmd.c b/src/cmd.c
> > > > index cb9e34c..12d8640 100644
> > > > --- a/src/cmd.c
> > > > +++ b/src/cmd.c
> > > > @@ -1463,38 +1463,44 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
> > > >  	return ret;
> > > >  }
> > > > 
> > > > -int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> > > > -		      struct ibv_ah_attr *attr)
> > > > +int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_attr *attr,
> > > > +			 struct ibv_create_ah *cmd, size_t cmd_size,
> > > > +			 struct ibv_create_ah_resp *resp, size_t resp_size)
> > > >  {
> > > > -	struct ibv_create_ah      cmd;
> > > > -	struct ibv_create_ah_resp resp;
> > > > -
> > > > -	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
> > > > -	cmd.user_handle            = (uintptr_t) ah;
> > > > -	cmd.pd_handle              = pd->handle;
> > > > -	cmd.attr.dlid              = attr->dlid;
> > > > -	cmd.attr.sl                = attr->sl;
> > > > -	cmd.attr.src_path_bits     = attr->src_path_bits;
> > > > -	cmd.attr.static_rate       = attr->static_rate;
> > > > -	cmd.attr.is_global         = attr->is_global;
> > > > -	cmd.attr.port_num          = attr->port_num;
> > > > -	cmd.attr.grh.flow_label    = attr->grh.flow_label;
> > > > -	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
> > > > -	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
> > > > -	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
> > > > -	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
> > > > +	IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size);
> > > You should start with a patch extending the kernel part while supporting
> > > backwards compatibility, later on when patch applied come with a
> > > matching user space patch for a review.
> > > 
> > > In addition, if you supply an extended command you should work with the
> > > extended macros for issuing a command to fully enable future extensions
> > > (e.g IBV_INIT_CMD_RESP_EX_V) and with a specific EX command as done in
> > > other cases (e.g CREATE_QP_EX).
> > 
> > Having looked at the details, I tend to think that what I am adding here is not
> > an extended command, it is more of a fix for a missing feature of
> > the original ibv_cmd_create_ah,
> 
> You want to extend the user->kernel API to get some extra data while 
> maintaining BW compatibility. Usually you need for that an extra command 
> which better be extended from day one. Can it be done in the kernel side 
> without a new command ? 

Yes, as we do not really change the core part of the protocol, we just
makes sure that provider specific fields can be transferred.

> please send pointer to the matching kernel patch.

This is the related patch, which basically allows drivers to make use of
the information sent from user space, by giving the create_ah callback access 
to udata, if available:

https://www.spinics.net/lists/linux-rdma/msg39879.html

> > Maybe I can rename it to ibv_cmd_create_ah_v2 (or v3?) to avoid associating it
> > with the extended command set?
> 
> In case you need a new command, let's add it in an extended mode so that 
> future extensions will use it instead of having later v3/v4, etc.

As I don't need a new command on the kernel side, 
I assume you mean v2 is ok, that way we avoid unnecessary cruft in my opinion.

Thanks,
Knut

--
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 libibverbs 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

On Thu, 2016-09-01 at 12:00 +0300, Yishai Hadas wrote:
> On 9/1/2016 9:59 AM, Knut Omang wrote:
> > Also proper end align the ibv_kern_send_wr struct.
> > 
> > Requires a corresponding kernel change.

https://www.spinics.net/lists/linux-rdma/msg39884.html

Knut

> 
> Again, usually starting from a kernel patch then coming with a matching 
> user one.
> 
> > Signed-off-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  include/infiniband/kern-abi.h | 1 +
> >  src/cmd.c                     | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
> > index 8bdeef5..7b1d310 100644
> > --- a/include/infiniband/kern-abi.h
> > +++ b/include/infiniband/kern-abi.h
> > @@ -800,6 +800,7 @@ struct ibv_kern_send_wr {
> >  	union {
> >  		struct {
> >  			__u32 remote_srqn;
> > +			__u32 reserved;
> >  		} xrc;
> >  	} qp_type;
> >  };
> > diff --git a/src/cmd.c b/src/cmd.c
> > index a418ee1..a4e2f75 100644
> > --- a/src/cmd.c
> > +++ b/src/cmd.c
> > @@ -1293,6 +1293,8 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
> >  			tmp->wr.ud.remote_qpn  = i->wr.ud.remote_qpn;
> >  			tmp->wr.ud.remote_qkey = i->wr.ud.remote_qkey;
> >  		} else {
> > +			if (ibqp->qp_type == IBV_QPT_XRC_SEND)
> > +				tmp->qp_type.xrc.remote_srqn = i->qp_type.xrc.remote_srqn;
> >  			switch (i->opcode) {
> >  			case IBV_WR_RDMA_WRITE:
> >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > 
> 
--
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