| Message ID | 1472713193-22397-3-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 user/kernel level API requires all parameter blocks to be > 64 bit end aligned. > > Also clean up some valgrind/memory initialization issues. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > --- > include/infiniband/kern-abi.h | 1 + > src/cmd.c | 5 ++++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h > index f70fa44..8bdeef5 100644 > --- a/include/infiniband/kern-abi.h > +++ b/include/infiniband/kern-abi.h > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp { > __u32 mr_handle; > __u32 lkey; > __u32 rkey; > + __u32 reserved; > }; > > struct ibv_rereg_mr { > diff --git a/src/cmd.c b/src/cmd.c > index 12d8640..a418ee1 100644 > --- a/src/cmd.c > +++ b/src/cmd.c > @@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att > IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size); > cmd->user_handle = (uintptr_t) ah; > cmd->pd_handle = pd->handle; > + cmd->reserved = 0; > cmd->attr.dlid = attr->dlid; > cmd->attr.sl = attr->sl; > cmd->attr.src_path_bits = attr->src_path_bits; > @@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att > cmd->attr.grh.sgid_index = attr->grh.sgid_index; > cmd->attr.grh.hop_limit = attr->grh.hop_limit; > cmd->attr.grh.traffic_class = attr->grh.traffic_class; > + cmd->attr.grh.reserved = 0; > + cmd->attr.reserved = 0; How those changes relate to this patch's subject ? seems that those and below relate to previous ibv_cmd_create_ah_ex patch. > memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16); > > if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size) > return errno; > > - (void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp); > + (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size); > > ah->handle = resp->handle; > ah->context = pd->context; > -- 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:56 +0300, Yishai Hadas wrote: > On 9/1/2016 9:59 AM, Knut Omang wrote: > > > > The user/kernel level API requires all parameter blocks to be > > 64 bit end aligned. > > > > Also clean up some valgrind/memory initialization issues. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > > --- > > include/infiniband/kern-abi.h | 1 + > > src/cmd.c | 5 ++++- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/include/infiniband/kern-abi.h > > b/include/infiniband/kern-abi.h > > index f70fa44..8bdeef5 100644 > > --- a/include/infiniband/kern-abi.h > > +++ b/include/infiniband/kern-abi.h > > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp { > > __u32 mr_handle; > > __u32 lkey; > > __u32 rkey; > > + __u32 reserved; > > }; > > > > struct ibv_rereg_mr { > > diff --git a/src/cmd.c b/src/cmd.c > > index 12d8640..a418ee1 100644 > > --- a/src/cmd.c > > +++ b/src/cmd.c > > @@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, > > struct ibv_ah *ah, struct ibv_ah_att > > IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, > > resp_size); > > cmd->user_handle = (uintptr_t) ah; > > cmd->pd_handle = pd->handle; > > + cmd->reserved = 0; > > cmd->attr.dlid = attr->dlid; > > cmd->attr.sl = attr->sl; > > cmd->attr.src_path_bits = attr->src_path_bits; > > @@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, > > struct ibv_ah *ah, struct ibv_ah_att > > cmd->attr.grh.sgid_index = attr->grh.sgid_index; > > cmd->attr.grh.hop_limit = attr->grh.hop_limit; > > cmd->attr.grh.traffic_class = attr->grh.traffic_class; > > + cmd->attr.grh.reserved = 0; > > + cmd->attr.reserved = 0; > How those changes relate to this patch's subject ? seems that those > and > below relate to previous ibv_cmd_create_ah_ex patch. These are fixing valgrind complaints in the original code, but because that code got rewritten by the previous patch, it might look that way. The initial idea was that this whole patch fixes trivial bugs/issues in the current libibverbs, while the other two provides new functionality. I can split this out as a separate patch for the valgrind part, if necessary. Knut > > > > > memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16); > > > > if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size) > > return errno; > > > > - (void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp); > > + (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size); > > > > ah->handle = resp->handle; > > ah->context = pd->context; > > -- 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:52AM +0200, Knut Omang wrote: > The user/kernel level API requires all parameter blocks to be > 64 bit end aligned. > > Also clean up some valgrind/memory initialization issues. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > include/infiniband/kern-abi.h | 1 + > src/cmd.c | 5 ++++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h > index f70fa44..8bdeef5 100644 > +++ b/include/infiniband/kern-abi.h > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp { > __u32 mr_handle; > __u32 lkey; > __u32 rkey; > + __u32 reserved; > }; This structure is a copy of include/uapi/rdma/ib_user_verbs.h, so you need to start with a kernel patch proposing this change. We will eventually get rid of kern-abi.h and use ib_user_verbs.h at some point. Maybe elaborate on why this is OK and doesn't break anything in that patch.. > - (void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp); > + (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size); Why the (void)? 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:42 -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 08:59:52AM +0200, Knut Omang wrote: > > The user/kernel level API requires all parameter blocks to be > > 64 bit end aligned. > > > > Also clean up some valgrind/memory initialization issues. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > Reviewed-by: Mukesh Kacker <mukesh.kacker@oracle.com> > > include/infiniband/kern-abi.h | 1 + > > src/cmd.c | 5 ++++- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h > > index f70fa44..8bdeef5 100644 > > +++ b/include/infiniband/kern-abi.h > > @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp { > > __u32 mr_handle; > > __u32 lkey; > > __u32 rkey; > > + __u32 reserved; > > }; > > This structure is a copy of include/uapi/rdma/ib_user_verbs.h, so you > need to start with a kernel patch proposing this change. Yes, I am aware of that, sorry - I intended to post both sets in sequence but ran into a few recently introduced conflicts in the kernel set, so still working on that (just testing remains) > We will eventually get rid of kern-abi.h and use ib_user_verbs.h at some > point. Good to get a common file for it and get rid of some copy/paste. > Maybe elaborate on why this is OK and doesn't break anything in that > patch.. > > > - (void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp); > > + (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size); > > Why the (void)? I guess it is there to avoid compiler warnings about unused variables in some configuration. I just kept it as it was in this patch. I see all instances throughout the code has this. 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
diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h index f70fa44..8bdeef5 100644 --- a/include/infiniband/kern-abi.h +++ b/include/infiniband/kern-abi.h @@ -367,6 +367,7 @@ struct ibv_reg_mr_resp { __u32 mr_handle; __u32 lkey; __u32 rkey; + __u32 reserved; }; struct ibv_rereg_mr { diff --git a/src/cmd.c b/src/cmd.c index 12d8640..a418ee1 100644 --- a/src/cmd.c +++ b/src/cmd.c @@ -1470,6 +1470,7 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att IBV_INIT_CMD_RESP(cmd, cmd_size, CREATE_AH, resp, resp_size); cmd->user_handle = (uintptr_t) ah; cmd->pd_handle = pd->handle; + cmd->reserved = 0; cmd->attr.dlid = attr->dlid; cmd->attr.sl = attr->sl; cmd->attr.src_path_bits = attr->src_path_bits; @@ -1480,12 +1481,14 @@ int ibv_cmd_create_ah_ex(struct ibv_pd *pd, struct ibv_ah *ah, struct ibv_ah_att cmd->attr.grh.sgid_index = attr->grh.sgid_index; cmd->attr.grh.hop_limit = attr->grh.hop_limit; cmd->attr.grh.traffic_class = attr->grh.traffic_class; + cmd->attr.grh.reserved = 0; + cmd->attr.reserved = 0; memcpy(cmd->attr.grh.dgid, attr->grh.dgid.raw, 16); if (write(pd->context->cmd_fd, cmd, cmd_size) != cmd_size) return errno; - (void) VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp); + (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size); ah->handle = resp->handle; ah->context = pd->context;