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@xxxxxxxxxx>
> > Reviewed-by: Mukesh Kacker <mukesh.kacker@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
