diff mbox

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

Message ID d551c3792e884443903c67212f505082cd53b135.1474352400.git-series.knut.omang@oracle.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Knut Omang Sept. 20, 2016, 6:22 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.4.

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          |  6 ++++-
 3 files changed, 37 insertions(+), 23 deletions(-)

Comments

Yishai Hadas Sept. 20, 2016, 11:17 a.m. UTC | #1
On 9/20/2016 9:22 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.
>
> The new call is versioned in the shared library as IBVERBS_1.4.
>
> 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          |  6 ++++-
>  3 files changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> index ea3dade..9356dc8 100644
> --- a/include/infiniband/driver.h
> +++ b/include/infiniband/driver.h
> @@ -234,6 +234,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 11f6509..58b9dcd 100644
> --- a/src/cmd.c
> +++ b/src/cmd.c
> @@ -1493,38 +1493,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 46744e5..ddba63a 100644
> --- a/src/libibverbs.map
> +++ b/src/libibverbs.map
> @@ -118,7 +118,6 @@ IBVERBS_1.1 {
>  		ibv_cmd_create_qp_ex2;
>  		ibv_cmd_open_qp;
>  		ibv_cmd_rereg_mr;
> -
>  } IBVERBS_1.0;
>
>  IBVERBS_1.3 {
> @@ -130,3 +129,8 @@ IBVERBS_1.3 {
>  		ibv_cmd_destroy_rwq_ind_table;
>  		ibv_query_gid_type;
>  } IBVERBS_1.1;
> +
> +IBVERBS_1.4 {

Any reason to add 1.4 instead of using 1.3 ? in my understanding the 
idea is that new symbols for next release will use a new ABI version, 
this is what 1.3 already supplied. Added Jason for his input as well.


> +	global:
> +		ibv_cmd_create_ah_ex;

When do you plan to contribute the driver code for that ? for now no 
upstream drivers uses this API, taking some code for future usage with 
no current usage is not the preferred way.

> +} IBVERBS_1.3;
>

--
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. 20, 2016, 11:37 a.m. UTC | #2
On Tue, 2016-09-20 at 14:17 +0300, Yishai Hadas wrote:
> On 9/20/2016 9:22 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.
> > 
> > The new call is versioned in the shared library as IBVERBS_1.4.
> > 
> > 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          |  6 ++++-
> >  3 files changed, 37 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > index ea3dade..9356dc8 100644
> > --- a/include/infiniband/driver.h
> > +++ b/include/infiniband/driver.h
> > @@ -234,6 +234,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 11f6509..58b9dcd 100644
> > --- a/src/cmd.c
> > +++ b/src/cmd.c
> > @@ -1493,38 +1493,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 46744e5..ddba63a 100644
> > --- a/src/libibverbs.map
> > +++ b/src/libibverbs.map
> > @@ -118,7 +118,6 @@ IBVERBS_1.1 {
> >  		ibv_cmd_create_qp_ex2;
> >  		ibv_cmd_open_qp;
> >  		ibv_cmd_rereg_mr;
> > -
> >  } IBVERBS_1.0;
> > 
> >  IBVERBS_1.3 {
> > @@ -130,3 +129,8 @@ IBVERBS_1.3 {
> >  		ibv_cmd_destroy_rwq_ind_table;
> >  		ibv_query_gid_type;
> >  } IBVERBS_1.1;
> > +
> > +IBVERBS_1.4 {
> Any reason to add 1.4 instead of using 1.3 ? in my understanding the 
> idea is that new symbols for next release will use a new ABI version, 
> this is what 1.3 already supplied. Added Jason for his input as well.

good point - Jason?

> > 
> > +	global:
> > +		ibv_cmd_create_ah_ex;
> When do you plan to contribute the driver code for that ? for now no 
> upstream drivers uses this API, taking some code for future usage with 
> no current usage is not the preferred way.

The plan is ASAP, the kernel side has been approved for release by Oracle's
legal dep, but the user library is still stuck, and with an undesirable
license combination given Jason's recent consolidation efforts :-/

Knut

> > 
> > +} IBVERBS_1.3;
> > 
--
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
Leon Romanovsky Sept. 20, 2016, 1:29 p.m. UTC | #3
On Tue, Sep 20, 2016 at 01:37:39PM +0200, Knut Omang wrote:
> On Tue, 2016-09-20 at 14:17 +0300, Yishai Hadas wrote:
> > On 9/20/2016 9:22 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.
> > >
> > > The new call is versioned in the shared library as IBVERBS_1.4.
> > >
> > > 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          |  6 ++++-
> > >  3 files changed, 37 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > > index ea3dade..9356dc8 100644
> > > --- a/include/infiniband/driver.h
> > > +++ b/include/infiniband/driver.h
> > > @@ -234,6 +234,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 11f6509..58b9dcd 100644
> > > --- a/src/cmd.c
> > > +++ b/src/cmd.c
> > > @@ -1493,38 +1493,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 46744e5..ddba63a 100644
> > > --- a/src/libibverbs.map
> > > +++ b/src/libibverbs.map
> > > @@ -118,7 +118,6 @@ IBVERBS_1.1 {
> > >  		ibv_cmd_create_qp_ex2;
> > >  		ibv_cmd_open_qp;
> > >  		ibv_cmd_rereg_mr;
> > > -
> > >  } IBVERBS_1.0;
> > >
> > >  IBVERBS_1.3 {
> > > @@ -130,3 +129,8 @@ IBVERBS_1.3 {
> > >  		ibv_cmd_destroy_rwq_ind_table;
> > >  		ibv_query_gid_type;
> > >  } IBVERBS_1.1;
> > > +
> > > +IBVERBS_1.4 {
> > Any reason to add 1.4 instead of using 1.3 ? in my understanding the 
> > idea is that new symbols for next release will use a new ABI version, 
> > this is what 1.3 already supplied. Added Jason for his input as well.
>
> good point - Jason?
>
> > >
> > > +	global:
> > > +		ibv_cmd_create_ah_ex;
> > When do you plan to contribute the driver code for that ? for now no 
> > upstream drivers uses this API, taking some code for future usage with 
> > no current usage is not the preferred way.
>
> The plan is ASAP, the kernel side has been approved for release by Oracle's
> legal dep, but the user library is still stuck, and with an undesirable
> license combination given Jason's recent consolidation efforts :-/

Jason didn't change licenses. You still can release your driver under
dual-license in similar way to other drivers.

>
> Knut
>
> > >
> > > +} IBVERBS_1.3;
> > >
> --
> 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. 20, 2016, 2:05 p.m. UTC | #4
On Tue, 2016-09-20 at 16:29 +0300, Leon Romanovsky wrote:
> On Tue, Sep 20, 2016 at 01:37:39PM +0200, Knut Omang wrote:
> > On Tue, 2016-09-20 at 14:17 +0300, Yishai Hadas wrote:
> > > On 9/20/2016 9:22 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.
> > > >
> > > > The new call is versioned in the shared library as IBVERBS_1.4.
> > > >
> > > > 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          |  6 ++++-
> > > >  3 files changed, 37 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
> > > > index ea3dade..9356dc8 100644
> > > > --- a/include/infiniband/driver.h
> > > > +++ b/include/infiniband/driver.h
> > > > @@ -234,6 +234,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 11f6509..58b9dcd 100644
> > > > --- a/src/cmd.c
> > > > +++ b/src/cmd.c
> > > > @@ -1493,38 +1493,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 46744e5..ddba63a 100644
> > > > --- a/src/libibverbs.map
> > > > +++ b/src/libibverbs.map
> > > > @@ -118,7 +118,6 @@ IBVERBS_1.1 {
> > > >  		ibv_cmd_create_qp_ex2;
> > > >  		ibv_cmd_open_qp;
> > > >  		ibv_cmd_rereg_mr;
> > > > -
> > > >  } IBVERBS_1.0;
> > > >
> > > >  IBVERBS_1.3 {
> > > > @@ -130,3 +129,8 @@ IBVERBS_1.3 {
> > > >  		ibv_cmd_destroy_rwq_ind_table;
> > > >  		ibv_query_gid_type;
> > > >  } IBVERBS_1.1;
> > > > +
> > > > +IBVERBS_1.4 {
> > > Any reason to add 1.4 instead of using 1.3 ? in my understanding the 
> > > idea is that new symbols for next release will use a new ABI version, 
> > > this is what 1.3 already supplied. Added Jason for his input as well.
> >
> > good point - Jason?
> >
> > > >
> > > > +	global:
> > > > +		ibv_cmd_create_ah_ex;
> > > When do you plan to contribute the driver code for that ? for now no 
> > > upstream drivers uses this API, taking some code for future usage with 
> > > no current usage is not the preferred way.
> >
> > The plan is ASAP, the kernel side has been approved for release by Oracle's
> > legal dep, but the user library is still stuck, and with an undesirable
> > license combination given Jason's recent consolidation efforts :-/
> 
> Jason didn't change licenses. You still can release your driver under
> dual-license in similar way to other drivers.

The problem is that libsif is stuck under a BSD only license imposed by 
the first set of lawyers involved, and to be compatible, we need it 
to be dual license GPLv2 + BSD like the rest of the provider libraries.

Knut

> >
> > Knut
> >
> > > >
> > > > +} IBVERBS_1.3;
> > > >
> > --
> > 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
Jason Gunthorpe Sept. 20, 2016, 3:32 p.m. UTC | #5
On Tue, Sep 20, 2016 at 02:17:04PM +0300, Yishai Hadas wrote:

> Any reason to add 1.4 instead of using 1.3 ? in my understanding the idea is
> that new symbols for next release will use a new ABI version, this is what
> 1.3 already supplied. Added Jason for his input as well.

Doug indicated he was going to roll a 1.3 release RSN, up to him what
number it will be eventually.
 
> >+	global:
> >+		ibv_cmd_create_ah_ex;
> 
> When do you plan to contribute the driver code for that ? for now no
> upstream drivers uses this API, taking some code for future usage with no
> current usage is not the preferred way.

Even a github reference with your pre-submission driver would be useful

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
Jason Gunthorpe Sept. 20, 2016, 3:51 p.m. UTC | #6
On Tue, Sep 20, 2016 at 04:05:50PM +0200, Knut Omang wrote:

> The problem is that libsif is stuck under a BSD only license imposed by 
> the first set of lawyers involved, and to be compatible, we need it 
> to be dual license GPLv2 + BSD like the rest of the provider libraries.

Here is some (not a lawyer) information you may find helpful.

Oracle is an OFA member and part of the purpose of groups of OFA is to
manage license issues across companies collaborating on the same code
base. Oracle will have agreed to various things on this topic when
they became a member. The OFA may be able to help you with your legal.

The OFA may choose to not distribute improperly licensed code in
OFED/etc.

GPLv2 compatability is important. Not just for the consolidated tree,
but for everyone. Distors will have complicated questions if asked to
ship a GPLv2 incompatible plugin at the same time as shipping a GPLv2
program that uses the plugin. You may find your module undistributed.

That said, it is widely regarded that the BSD 2 and 3 clause license
varients are GPLv2 compatible on their own without any additional
language.

My view on the OFA dual license scheme is that it is used to
provide absolute certainty that the code is GPLv2 compatible.
Many other projects rely on the compatibility without being explicit.

I would have no objection to a BSD 2/3 clause licensed provider in the
consolidated tree.  (while being confused why adding the GPLv2 option
is such a difficult problem, and re-emphasising the view that the code
will be used and distributed under the GPLv2 in some situations.)

A GPLv2 incompatible option like the CDDL, Apache license, etc is not
acceptable. Do not get creative and add new clauses to the stanard BSD
license.

So, in short, I would urge you to work with your legal to use the OFA
dual license. If that is completely impossible then a standard BSD
license will work to some degree. Remember, it is very hard to change
licenses after the fact.

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. 22, 2016, 4:35 a.m. UTC | #7
On Tue, 2016-09-20 at 09:51 -0600, Jason Gunthorpe wrote:
> On Tue, Sep 20, 2016 at 04:05:50PM +0200, Knut Omang wrote:
> 
> > The problem is that libsif is stuck under a BSD only license imposed by 
> > the first set of lawyers involved, and to be compatible, we need it 
> > to be dual license GPLv2 + BSD like the rest of the provider libraries.
> 
> Here is some (not a lawyer) information you may find helpful.
> 
> Oracle is an OFA member and part of the purpose of groups of OFA is to
> manage license issues across companies collaborating on the same code
> base. Oracle will have agreed to various things on this topic when
> they became a member. The OFA may be able to help you with your legal.
> 
> The OFA may choose to not distribute improperly licensed code in
> OFED/etc.
> 
> GPLv2 compatability is important. Not just for the consolidated tree,
> but for everyone. Distors will have complicated questions if asked to
> ship a GPLv2 incompatible plugin at the same time as shipping a GPLv2
> program that uses the plugin. You may find your module undistributed.
> 
> That said, it is widely regarded that the BSD 2 and 3 clause license
> varients are GPLv2 compatible on their own without any additional
> language.
> 
> My view on the OFA dual license scheme is that it is used to
> provide absolute certainty that the code is GPLv2 compatible.
> Many other projects rely on the compatibility without being explicit.
> 
> I would have no objection to a BSD 2/3 clause licensed provider in the
> consolidated tree.  (while being confused why adding the GPLv2 option
> is such a difficult problem, and re-emphasising the view that the code
> will be used and distributed under the GPLv2 in some situations.)
> 
> A GPLv2 incompatible option like the CDDL, Apache license, etc is not
> acceptable. Do not get creative and add new clauses to the stanard BSD
> license.
> 
> So, in short, I would urge you to work with your legal to use the OFA
> dual license. If that is completely impossible then a standard BSD
> license will work to some degree. Remember, it is very hard to change
> licenses after the fact.

Thanks for spending time on this, Jason, appreciated!
I'll bring it forward as best as I can. The point of piggybacking on the OFA
work on licensing is a good point I'll forward to the legal dep.
And this discussion also reinforces my argument that we, the SIF software team 
should persist in our request to get this right from the beginning.

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
--
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. 22, 2016, 4:46 a.m. UTC | #8
On Tue, 2016-09-20 at 09:32 -0600, Jason Gunthorpe wrote:
> On Tue, Sep 20, 2016 at 02:17:04PM +0300, Yishai Hadas wrote:
> 
> > Any reason to add 1.4 instead of using 1.3 ? in my understanding the idea is
> > that new symbols for next release will use a new ABI version, this is what
> > 1.3 already supplied. Added Jason for his input as well.
> 
> Doug indicated he was going to roll a 1.3 release RSN, up to him what
> number it will be eventually.
>  
> > >+	global:
> > >+		ibv_cmd_create_ah_ex;
> > 
> > When do you plan to contribute the driver code for that ? for now no
> > upstream drivers uses this API, taking some code for future usage with no
> > current usage is not the preferred way.
> 
> Even a github reference with your pre-submission driver would be useful

Tried to find an Oracle employee active user without success so far, 
so I need to validate that this is ok. In parallel I am pursuing getting 
public git repos on the Oracle OSS site for it.

My plan is to post the driver in a big
file-by-file commit series rebased to the latest, but I have 
got a bit delayed by other work, so I think realistically 
that I am not ready until mid-Oct.

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
--
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 ea3dade..9356dc8 100644
--- a/include/infiniband/driver.h
+++ b/include/infiniband/driver.h
@@ -234,6 +234,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 11f6509..58b9dcd 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1493,38 +1493,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 46744e5..ddba63a 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -118,7 +118,6 @@  IBVERBS_1.1 {
 		ibv_cmd_create_qp_ex2;
 		ibv_cmd_open_qp;
 		ibv_cmd_rereg_mr;
-
 } IBVERBS_1.0;
 
 IBVERBS_1.3 {
@@ -130,3 +129,8 @@  IBVERBS_1.3 {
 		ibv_cmd_destroy_rwq_ind_table;
 		ibv_query_gid_type;
 } IBVERBS_1.1;
+
+IBVERBS_1.4 {
+	global:
+		ibv_cmd_create_ah_ex;
+} IBVERBS_1.3;