[PATCH libibverbs v2 0/3] SIF related libibverbs patches

[PATCH libibverbs v2 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.

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

Patch 1 introduces a new ibv_cmd_create_ah_ex under a new .so file version
IBVERBS_1.3 in order to preserve backward binary compatibility with older
provider builds.

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

Patch 3 is similar to patch 2 except that no provider libs uses this code path
(kernel supported XRC send) until now, so no extra backward compatibility measures
are needed.

Changes since v1:
  - Introduce the new ibv_cmd_create_ah_ex as a new version IBVERBS_1.3 in the
    map file.

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            |  4 +++-
 4 files changed, 43 insertions(+), 22 deletions(-)

base-commit: 0be978ea2bfaf203c35334b090bddb280de62611
-- 
git-series 0.8.10
--
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 v2 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.

The new call is versioned in the shared library as IBVERBS_1.3
to leave room for a 1.2 that should have been added for other changes.

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          |  4 +++-
 3 files changed, 36 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..e327a84 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -118,5 +118,9 @@ IBVERBS_1.1 {
 		ibv_cmd_create_qp_ex2;
 		ibv_cmd_open_qp;
 		ibv_cmd_rereg_mr;
+};
 
+IBVERBS_1.3 {
+	global:
+		ibv_cmd_create_ah_ex;
 } IBVERBS_1.0;
-- 
git-series 0.8.10
--
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 v2 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;
-- 
git-series 0.8.10
--
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 v2 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(+), 0 deletions(-)

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:
-- 
git-series 0.8.10
--
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 v2 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Sat, Sep 17, 2016 at 05:59:11AM +0200, Knut Omang wrote:
> @@ -118,5 +118,9 @@ IBVERBS_1.1 {
>  		ibv_cmd_create_qp_ex2;
>  		ibv_cmd_open_qp;
>  		ibv_cmd_rereg_mr;
> +};
>  
> +IBVERBS_1.3 {
> +	global:
> +		ibv_cmd_create_ah_ex;
>  } IBVERBS_1.0;

This will probably end up being 1.4, and the 'tree' structure is
wrong, do not move this line:

>  } IBVERBS_1.0;

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

[Knut Omang]

On Sun, 2016-09-18 at 21:01 -0600, Jason Gunthorpe wrote:
> On Sat, Sep 17, 2016 at 05:59:11AM +0200, Knut Omang wrote:
> > @@ -118,5 +118,9 @@ IBVERBS_1.1 {
> >  		ibv_cmd_create_qp_ex2;
> >  		ibv_cmd_open_qp;
> >  		ibv_cmd_rereg_mr;
> > +};
> >  
> > +IBVERBS_1.3 {
> > +	global:
> > +		ibv_cmd_create_ah_ex;
> >  } IBVERBS_1.0;
> 
> This will probably end up being 1.4, and the 'tree' structure is
> wrong, do not move this line:
> 
> >  } IBVERBS_1.0;

I was a bit concerned about the "structure", but ultimately decided to
follow the pattern since I could not find any documentation with this syntax
(which is probably because it is not supported?)

As far as I can see, the final IBVERBS_1.0 does not have any effect at all, 
the symbol output seems identical, so it should probably be removed, but
maybe as a separate patch, with explanation?

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

[Knut Omang]

On Mon, 2016-09-19 at 05:51 +0200, Knut Omang wrote:
> On Sun, 2016-09-18 at 21:01 -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 17, 2016 at 05:59:11AM +0200, Knut Omang wrote:
> > > @@ -118,5 +118,9 @@ IBVERBS_1.1 {
> > >  		ibv_cmd_create_qp_ex2;
> > >  		ibv_cmd_open_qp;
> > >  		ibv_cmd_rereg_mr;
> > > +};
> > >  
> > > +IBVERBS_1.3 {
> > > +	global:
> > > +		ibv_cmd_create_ah_ex;
> > >  } IBVERBS_1.0;
> > 
> > This will probably end up being 1.4, and the 'tree' structure is
> > wrong, do not move this line:
> > 
> > >  } IBVERBS_1.0;
> 
> I was a bit concerned about the "structure", but ultimately decided to
> follow the pattern since I could not find any documentation with this syntax
> (which is probably because it is not supported?)

Without digging into the details of the ld map parser, my hunch is that 
a version symbol optionally accepts the {}, and that the ; terminates it so
as far as ld is concerned, basically the final IBVERBS_1.0 
just provides an empty list of additional symbols for IBVERBS_1.0

> As far as I can see, the final IBVERBS_1.0 does not have any effect at all, 
> the symbol output seems identical, so it should probably be removed, but
> maybe as a separate patch, with explanation?
> 
> 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 v2 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Mon, Sep 19, 2016 at 05:51:03AM +0200, Knut Omang wrote:

> I was a bit concerned about the "structure", but ultimately decided to
> follow the pattern since I could not find any documentation with this syntax
> (which is probably because it is not supported?)

It is something carried over from Solaris, it is not implemented in
linux today, but it is good practice to follow the solaris
method. Drepper talks about it in his various documentations of symbol
versioning.

Each stanza should refer to its predecessor.

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

[Knut Omang]

On Sun, 2016-09-18 at 22:08 -0600, Jason Gunthorpe wrote:
> On Mon, Sep 19, 2016 at 05:51:03AM +0200, Knut Omang wrote:
> 
> > I was a bit concerned about the "structure", but ultimately decided to
> > follow the pattern since I could not find any documentation with this syntax
> > (which is probably because it is not supported?)
> 
> It is something carried over from Solaris, it is not implemented in
> linux today, but it is good practice to follow the solaris
> method. Drepper talks about it in his various documentations of symbol
> versioning.
> 
> Each stanza should refer to its predecessor.

Ok, I see...
That also means that this patch would have to be based on the IBVERBS_1.2 and IBVERBS_1.3 fixups
to avoid introducing conflicts.

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

[Knut Omang]

On Mon, 2016-09-19 at 06:29 +0200, Knut Omang wrote:
> On Sun, 2016-09-18 at 22:08 -0600, Jason Gunthorpe wrote:
> > On Mon, Sep 19, 2016 at 05:51:03AM +0200, Knut Omang wrote:
> > 
> > > I was a bit concerned about the "structure", but ultimately decided to
> > > follow the pattern since I could not find any documentation with this syntax
> > > (which is probably because it is not supported?)
> > 
> > It is something carried over from Solaris, it is not implemented in
> > linux today, but it is good practice to follow the solaris
> > method. Drepper talks about it in his various documentations of symbol
> > versioning.
> > 
> > Each stanza should refer to its predecessor.
> 
> Ok, I see...
> That also means that this patch would have to be based on the IBVERBS_1.2 and IBVERBS_1.3 fixups
> to avoid introducing conflicts.

I tested this, which seems to work fine, but I am not sure you want it ;-) :

diff --git a/src/libibverbs.map b/src/libibverbs.map
index 5134bd9..979980d 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -118,5 +118,15 @@ IBVERBS_1.1 {
                ibv_cmd_create_qp_ex2;
                ibv_cmd_open_qp;
                ibv_cmd_rereg_mr;
 } IBVERBS_1.0;

+IBVERBS_1.2 {
+} IBVERBS_1.1;
+
+
+IBVERBS_1.3 {
+} IBVERBS_1.2;
+
+IBVERBS_1.4 {
+ global:
+         ibv_cmd_create_ah_ex;
+} IBVERBS_1.3;


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

[Leon Romanovsky]

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

On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> 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(+), 0 deletions(-)
>
> 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;

It will be checked for all QPTs and for all consumers. Any chances to
optimize it?

>  			switch (i->opcode) {
>  			case IBV_WR_RDMA_WRITE:
>  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> --
> git-series 0.8.10
> --
> 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

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

Re: [PATCH libibverbs v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > 
> > 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(+), 0 deletions(-)
> > 
> > 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;
> It will be checked for all QPTs and for all consumers. Any chances to
> optimize it?

All QPTs except UD, yes.

The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
as long as the qp_type union is not used for anything else, but is it worth 
the loss of intuitiveness/future potential correctness of the code?

Note that this is for user mode post_send implemented by the kernel, not the "fast path"
pure user mode.

Knut

> > 
> >  			switch (i->opcode) {
> >  			case IBV_WR_RDMA_WRITE:
> >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > --
> > git-series 0.8.10
> > --
> > 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 v2 1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

[Jason Gunthorpe]

On Mon, Sep 19, 2016 at 06:36:32AM +0200, Knut Omang wrote:
> diff --git a/src/libibverbs.map b/src/libibverbs.map
> index 5134bd9..979980d 100644
> +++ b/src/libibverbs.map
> @@ -118,5 +118,15 @@ IBVERBS_1.1 {
>                 ibv_cmd_create_qp_ex2;
>                 ibv_cmd_open_qp;
>                 ibv_cmd_rereg_mr;
>  } IBVERBS_1.0;
> 
> +IBVERBS_1.2 {
> +} IBVERBS_1.1;

This is the right idea, but we won't be going back to fix the bad tags
from 1.2 and other releases so this can be dropped.

> +IBVERBS_1.3 {
> +} IBVERBS_1.2;

If you pull from Doug's tree right now you'll get a populated version
of this stanza

> +IBVERBS_1.4 {
> + global:
> +         ibv_cmd_create_ah_ex;
> +} IBVERBS_1.3;

Right. Doug will have to figure out a process for these, eg if he
hasn't released 1.3 yet then this can be folded into 1.3.

It is critical that once a release is made that the existing stanza's
not be changed.

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

[Knut Omang]

On Mon, 2016-09-19 at 10:59 -0600, Jason Gunthorpe wrote:
> On Mon, Sep 19, 2016 at 06:36:32AM +0200, Knut Omang wrote:
> > diff --git a/src/libibverbs.map b/src/libibverbs.map
> > index 5134bd9..979980d 100644
> > +++ b/src/libibverbs.map
> > @@ -118,5 +118,15 @@ IBVERBS_1.1 {
> >                 ibv_cmd_create_qp_ex2;
> >                 ibv_cmd_open_qp;
> >                 ibv_cmd_rereg_mr;
> >  } IBVERBS_1.0;
> > 
> > +IBVERBS_1.2 {
> > +} IBVERBS_1.1;
> 
> This is the right idea, but we won't be going back to fix the bad tags
> from 1.2 and other releases so this can be dropped.
> 
> > +IBVERBS_1.3 {
> > +} IBVERBS_1.2;
> 
> If you pull from Doug's tree right now you'll get a populated version
> of this stanza
> 
> > +IBVERBS_1.4 {
> > + global:
> > +         ibv_cmd_create_ah_ex;
> > +} IBVERBS_1.3;
> 
> Right. Doug will have to figure out a process for these, eg if he
> hasn't released 1.3 yet then this can be folded into 1.3.
> 
> It is critical that once a release is made that the existing stanza's
> not be changed.

Ok, fine - I see no 1.3.x tag yet so will post av v3 with this fix now.

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

[Leon Romanovsky]

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

On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > >
> > > 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(+), 0 deletions(-)
> > >
> > > 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;
> > It will be checked for all QPTs and for all consumers. Any chances to
> > optimize it?
>
> All QPTs except UD, yes.
>
> The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> as long as the qp_type union is not used for anything else, but is it worth 
> the loss of intuitiveness/future potential correctness of the code?

Let's concentrate on the present and as far as I see there are no kernel users who
need this field xrc.remote_srqn field.

>
> Note that this is for user mode post_send implemented by the kernel, not the "fast path"
> pure user mode.
>
> Knut
>
> > >
> > >  			switch (i->opcode) {
> > >  			case IBV_WR_RDMA_WRITE:
> > >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > > --
> > > git-series 0.8.10
> > > --
> > > 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

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

Re: [PATCH libibverbs v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

On Tue, 2016-09-20 at 13:18 +0300, Leon Romanovsky wrote:
> On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> > 
> > On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > > 
> > > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > > > 
> > > > 
> > > > 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(+), 0 deletions(-)
> > > > 
> > > > 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;
> > > It will be checked for all QPTs and for all consumers. Any chances to
> > > optimize it?
> > All QPTs except UD, yes.
> > 
> > The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> > as long as the qp_type union is not used for anything else, but is it worth 
> > the loss of intuitiveness/future potential correctness of the code?
> Let's concentrate on the present and as far as I see there are no kernel users who
> need this field xrc.remote_srqn field.

So I assume then that you are ok with this code, given my explanation?

Thanks,
Knut

> > 
> > 
> > Note that this is for user mode post_send implemented by the kernel, not the "fast path"
> > pure user mode.
> > 
> > Knut
> > 
> > > 
> > > > 
> > > > 
> > > >  			switch (i->opcode) {
> > > >  			case IBV_WR_RDMA_WRITE:
> > > >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > > > --
> > > > git-series 0.8.10
> > > > --
> > > > 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 v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Leon Romanovsky]

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

On Tue, Sep 20, 2016 at 12:43:30PM +0200, Knut Omang wrote:
> On Tue, 2016-09-20 at 13:18 +0300, Leon Romanovsky wrote:
> > On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> > >
> > > On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > > >
> > > > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > > > >
> > > > >
> > > > > 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(+), 0 deletions(-)
> > > > >
> > > > > 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;
> > > > It will be checked for all QPTs and for all consumers. Any chances to
> > > > optimize it?
> > > All QPTs except UD, yes.
> > >
> > > The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> > > as long as the qp_type union is not used for anything else, but is it worth 
> > > the loss of intuitiveness/future potential correctness of the code?
> > Let's concentrate on the present and as far as I see there are no kernel users who
> > need this field xrc.remote_srqn field.
>
> So I assume then that you are ok with this code, given my explanation?

Partially,

I'm fine with the claim about readability and the fact that
this code is not in data-path relaxed me a little bit.

I'm not fine with the part that this code doesn't do anything in kernel.

>
> Thanks,
> Knut
>
> > >
> > >
> > > Note that this is for user mode post_send implemented by the kernel, not the "fast path"
> > > pure user mode.
> > >
> > > Knut
> > >
> > > >
> > > > >
> > > > >
> > > > >  			switch (i->opcode) {
> > > > >  			case IBV_WR_RDMA_WRITE:
> > > > >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > > > > --
> > > > > git-series 0.8.10
> > > > > --
> > > > > 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

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

Re: [PATCH libibverbs v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

On Tue, 2016-09-20 at 14:01 +0300, Leon Romanovsky wrote:
> On Tue, Sep 20, 2016 at 12:43:30PM +0200, Knut Omang wrote:
> > 
> > On Tue, 2016-09-20 at 13:18 +0300, Leon Romanovsky wrote:
> > > 
> > > On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> > > > 
> > > > 
> > > > On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > > > > 
> > > > > 
> > > > > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 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(+), 0 deletions(-)
> > > > > > 
> > > > > > 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;
> > > > > It will be checked for all QPTs and for all consumers. Any chances to
> > > > > optimize it?
> > > > All QPTs except UD, yes.
> > > > 
> > > > The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> > > > as long as the qp_type union is not used for anything else, but is it worth 
> > > > the loss of intuitiveness/future potential correctness of the code?
> > > Let's concentrate on the present and as far as I see there are no kernel users who
> > > need this field xrc.remote_srqn field.
> > So I assume then that you are ok with this code, given my explanation?
> Partially,
> 
> I'm fine with the claim about readability and the fact that
> this code is not in data-path relaxed me a little bit.

Good!

> I'm not fine with the part that this code doesn't do anything in kernel.

I know, I have two big patch sets one kernel and one for the new "rdma consolidated" 
in the queue that should fix that, just want to save a few versions of those 
~32000 lines of code by preparing the baseline :-)

Knut

> 
> > 
> > 
> > Thanks,
> > Knut
> > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > Note that this is for user mode post_send implemented by the kernel, not the "fast path"
> > > > pure user mode.
> > > > 
> > > > Knut
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  			switch (i->opcode) {
> > > > > >  			case IBV_WR_RDMA_WRITE:
> > > > > >  			case IBV_WR_RDMA_WRITE_WITH_IMM:
> > > > > > --
> > > > > > git-series 0.8.10
> > > > > > --
> > > > > > 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 v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Leon Romanovsky]

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

On Tue, Sep 20, 2016 at 01:08:35PM +0200, Knut Omang wrote:
> On Tue, 2016-09-20 at 14:01 +0300, Leon Romanovsky wrote:
> > On Tue, Sep 20, 2016 at 12:43:30PM +0200, Knut Omang wrote:
> > >
> > > On Tue, 2016-09-20 at 13:18 +0300, Leon Romanovsky wrote:
> > > >
> > > > On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> > > > >
> > > > >
> > > > > On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > > > > >
> > > > > >
> > > > > > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 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(+), 0 deletions(-)
> > > > > > >
> > > > > > > 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;
> > > > > > It will be checked for all QPTs and for all consumers. Any chances to
> > > > > > optimize it?
> > > > > All QPTs except UD, yes.
> > > > >
> > > > > The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> > > > > as long as the qp_type union is not used for anything else, but is it worth 
> > > > > the loss of intuitiveness/future potential correctness of the code?
> > > > Let's concentrate on the present and as far as I see there are no kernel users who
> > > > need this field xrc.remote_srqn field.
> > > So I assume then that you are ok with this code, given my explanation?
> > Partially,
> >
> > I'm fine with the claim about readability and the fact that
> > this code is not in data-path relaxed me a little bit.
>
> Good!
>
> > I'm not fine with the part that this code doesn't do anything in kernel.
>
> I know, I have two big patch sets one kernel and one for the new "rdma consolidated" 
> in the queue that should fix that, just want to save a few versions of those 
> ~32000 lines of code by preparing the baseline :-)

It won't save. We DO believe you that this this new addition to UAPI is really
needed, but we NEED to see that it is unavoidable.

The proper flow for submission is driver->kernel_core->libraries and not
kernel_core->libraries->driver.

Thanks

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

Re: [PATCH libibverbs v2 3/3] Provide remote XRC SRQ number in kernel post_send.

[Knut Omang]

On Tue, 2016-09-20 at 15:17 +0300, Leon Romanovsky wrote:
> On Tue, Sep 20, 2016 at 01:08:35PM +0200, Knut Omang wrote:
> > On Tue, 2016-09-20 at 14:01 +0300, Leon Romanovsky wrote:
> > > On Tue, Sep 20, 2016 at 12:43:30PM +0200, Knut Omang wrote:
> > > >
> > > > On Tue, 2016-09-20 at 13:18 +0300, Leon Romanovsky wrote:
> > > > >
> > > > > On Mon, Sep 19, 2016 at 11:12:36AM +0200, Knut Omang wrote:
> > > > > >
> > > > > >
> > > > > > On Mon, 2016-09-19 at 08:29 +0300, Leon Romanovsky wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Sat, Sep 17, 2016 at 05:59:13AM +0200, Knut Omang wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > 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(+), 0 deletions(-)
> > > > > > > >
> > > > > > > > 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;
> > > > > > > It will be checked for all QPTs and for all consumers. Any chances to
> > > > > > > optimize it?
> > > > > > All QPTs except UD, yes.
> > > > > >
> > > > > > The easiest is perhaps just to remove the test (and set the attribute in all cases), which will work fine
> > > > > > as long as the qp_type union is not used for anything else, but is it worth 
> > > > > > the loss of intuitiveness/future potential correctness of the code?
> > > > > Let's concentrate on the present and as far as I see there are no kernel users who
> > > > > need this field xrc.remote_srqn field.
> > > > So I assume then that you are ok with this code, given my explanation?
> > > Partially,
> > >
> > > I'm fine with the claim about readability and the fact that
> > > this code is not in data-path relaxed me a little bit.
> >
> > Good!
> >
> > > I'm not fine with the part that this code doesn't do anything in kernel.
> >
> > I know, I have two big patch sets one kernel and one for the new "rdma consolidated" 
> > in the queue that should fix that, just want to save a few versions of those 
> > ~32000 lines of code by preparing the baseline :-)
> 
> It won't save. We DO believe you that this this new addition to UAPI is really
> needed, but we NEED to see that it is unavoidable.
> 
> The proper flow for submission is driver->kernel_core->libraries and not
> kernel_core->libraries->driver.

Ok, got it..

Knut

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