| Message ID | 1472713193-22397-2-git-send-email-knut.omang@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@oracle.com> > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> > > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> > > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > > --- > > 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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> >>> Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> >>> --- >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> > > > > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > > > > --- > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;