diff mbox

[libibverbs,v2,1/3] Add new call ibv_cmd_create_ah_ex which supports extra parameters

Message ID 5cc0157c0f05701d8cb1334e6ee11e1e7be6fa24.1474063039.git-series.knut.omang@oracle.com (mailing list archive)
State Superseded
Headers show

Commit Message

Knut Omang Sept. 17, 2016, 3:59 a.m. UTC 
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@oracle.com>
Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com>
---
 include/infiniband/driver.h |  4 +++-
 src/cmd.c                   | 50 +++++++++++++++++++++-----------------
 src/libibverbs.map          |  4 +++-
 3 files changed, 36 insertions(+), 22 deletions(-)

Comments

Jason Gunthorpe Sept. 19, 2016, 3:01 a.m. UTC | #1 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Knut Omang Sept. 19, 2016, 3:51 a.m. UTC | #2 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Knut Omang Sept. 19, 2016, 4:03 a.m. UTC | #3 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Sept. 19, 2016, 4:08 a.m. UTC | #4 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Knut Omang Sept. 19, 2016, 4:29 a.m. UTC | #5 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;