| Message ID | 20181014061118.14889-3-shamir.rabinovitch@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | convey ib_ucontext via ib_udata | expand |
On Sun, Oct 14, 2018 at 09:11:13AM +0300, Shamir Rabinovitch wrote: > initialize the context field in ib_udata created from uverbs commands > and extended commands path. > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > drivers/infiniband/core/uverbs.h | 9 ++++--- > drivers/infiniband/core/uverbs_cmd.c | 35 ++++++++++++++------------- > drivers/infiniband/core/uverbs_main.c | 6 +++-- > 3 files changed, 28 insertions(+), 22 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h > index c97935a0c7c6..3215f2f4e6d7 100644 > +++ b/drivers/infiniband/core/uverbs.h > @@ -55,23 +55,26 @@ static inline void > ib_uverbs_init_udata(struct ib_udata *udata, > const void __user *ibuf, > void __user *obuf, > - size_t ilen, size_t olen) > + size_t ilen, size_t olen, > + struct ib_ucontext *context) > { > udata->inbuf = ibuf; > udata->outbuf = obuf; > udata->inlen = ilen; > udata->outlen = olen; > + udata->context = context; > } > > static inline void > ib_uverbs_init_udata_buf_or_null(struct ib_udata *udata, > const void __user *ibuf, > void __user *obuf, > - size_t ilen, size_t olen) > + size_t ilen, size_t olen, > + struct ib_ucontext *context) > { > ib_uverbs_init_udata(udata, > ilen ? ibuf : NULL, olen ? obuf : NULL, > - ilen, olen); > + ilen, olen, context); > } > > /* > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index d41497acb264..cfe35ba0b619 100644 > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -100,7 +100,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, > ib_uverbs_init_udata(&udata, buf + sizeof(cmd), > u64_to_user_ptr(cmd.response) + sizeof(resp), > in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), > - out_len - sizeof(resp)); > + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); > ret = ib_rdmacg_try_charge(&cg_obj, ib_dev, RDMACG_RESOURCE_HCA_HANDLE); > if (ret) > @@ -358,7 +358,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file, > ib_uverbs_init_udata(&udata, buf + sizeof(cmd), > u64_to_user_ptr(cmd.response) + sizeof(resp), > in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), > - out_len - sizeof(resp)); > + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); No, this has to come form the uobject, get_ucontext is expensive and can return NULL unexpectedly in this context.. This is why I keep saying it must be set during uobj_get/etc. Jason
On Sun, Oct 14, 2018 at 01:22:28PM -0600, Jason Gunthorpe wrote: > On Sun, Oct 14, 2018 at 09:11:13AM +0300, Shamir Rabinovitch wrote: > > initialize the context field in ib_udata created from uverbs commands > > and extended commands path. > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > drivers/infiniband/core/uverbs.h | 9 ++++--- > > drivers/infiniband/core/uverbs_cmd.c | 35 ++++++++++++++------------- > > drivers/infiniband/core/uverbs_main.c | 6 +++-- > > 3 files changed, 28 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h > > index c97935a0c7c6..3215f2f4e6d7 100644 > > +++ b/drivers/infiniband/core/uverbs.h > > @@ -55,23 +55,26 @@ static inline void > > ib_uverbs_init_udata(struct ib_udata *udata, > > const void __user *ibuf, > > void __user *obuf, > > - size_t ilen, size_t olen) > > + size_t ilen, size_t olen, > > + struct ib_ucontext *context) > > { > > udata->inbuf = ibuf; > > udata->outbuf = obuf; > > udata->inlen = ilen; > > udata->outlen = olen; > > + udata->context = context; > > } > > > > static inline void > > ib_uverbs_init_udata_buf_or_null(struct ib_udata *udata, > > const void __user *ibuf, > > void __user *obuf, > > - size_t ilen, size_t olen) > > + size_t ilen, size_t olen, > > + struct ib_ucontext *context) > > { > > ib_uverbs_init_udata(udata, > > ilen ? ibuf : NULL, olen ? obuf : NULL, > > - ilen, olen); > > + ilen, olen, context); > > } > > > > /* > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > > index d41497acb264..cfe35ba0b619 100644 > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > @@ -100,7 +100,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, > > ib_uverbs_init_udata(&udata, buf + sizeof(cmd), > > u64_to_user_ptr(cmd.response) + sizeof(resp), > > in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), > > - out_len - sizeof(resp)); > > + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); > > > ret = ib_rdmacg_try_charge(&cg_obj, ib_dev, RDMACG_RESOURCE_HCA_HANDLE); > > if (ret) > > @@ -358,7 +358,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file, > > ib_uverbs_init_udata(&udata, buf + sizeof(cmd), > > u64_to_user_ptr(cmd.response) + sizeof(resp), > > in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), > > - out_len - sizeof(resp)); > > + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); > > No, this has to come form the uobject, get_ucontext is expensive and > can return NULL unexpectedly in this context.. > > This is why I keep saying it must be set during uobj_get/etc. > > Jason uobj_get_xx are used to get the sub objects of the prime object. Using this to copy the context to the udata can result in copying the context over and over for prime object that has couple of sub objects. Each of the sub objects is expected to have same context as the prime object. I have series that take the context from the prime object context and when we do not have such object we just initialize the udata context to NULL. If this is OK with you, I'd like to post the series. The series fix the kbuild hns build issues and I can send it as one long series (avoid kbuild detecting missing function on patch-set #2) or as 3 patch-sets for easy review. Thanks, Shamir
On Mon, Oct 29, 2018 at 10:22:11PM +0200, Shamir Rabinovitch wrote: > uobj_get_xx are used to get the sub objects of the prime object. > Using this to copy the context to the udata can result in copying > the context over and over for prime object that has couple of sub > objects. Each of the sub objects is expected to have same context > as the prime object. I don't think this really matters, we have few places like this. > I have series that take the context from the prime object context > and when we do not have such object we just initialize the udata > context to NULL. > The series fix the kbuild hns build issues and I can send it as one > long series (avoid kbuild detecting missing function on patch-set #2) > or as 3 patch-sets for easy review. You don't need to keep re-posting the other two series. It is clear the intent of this series, so let's get this done then you can post the revised version of the next series, etc. Jason
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h index c97935a0c7c6..3215f2f4e6d7 100644 --- a/drivers/infiniband/core/uverbs.h +++ b/drivers/infiniband/core/uverbs.h @@ -55,23 +55,26 @@ static inline void ib_uverbs_init_udata(struct ib_udata *udata, const void __user *ibuf, void __user *obuf, - size_t ilen, size_t olen) + size_t ilen, size_t olen, + struct ib_ucontext *context) { udata->inbuf = ibuf; udata->outbuf = obuf; udata->inlen = ilen; udata->outlen = olen; + udata->context = context; } static inline void ib_uverbs_init_udata_buf_or_null(struct ib_udata *udata, const void __user *ibuf, void __user *obuf, - size_t ilen, size_t olen) + size_t ilen, size_t olen, + struct ib_ucontext *context) { ib_uverbs_init_udata(udata, ilen ? ibuf : NULL, olen ? obuf : NULL, - ilen, olen); + ilen, olen, context); } /* diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index d41497acb264..cfe35ba0b619 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -100,7 +100,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); ret = ib_rdmacg_try_charge(&cg_obj, ib_dev, RDMACG_RESOURCE_HCA_HANDLE); if (ret) @@ -358,7 +358,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); uobj = uobj_alloc(UVERBS_OBJECT_PD, file, &ib_dev); if (IS_ERR(uobj)) @@ -518,7 +518,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); mutex_lock(&file->device->xrcd_tree_mutex); @@ -675,7 +675,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); if ((cmd.start & ~PAGE_MASK) != (cmd.hca_va & ~PAGE_MASK)) return -EINVAL; @@ -767,7 +767,7 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); if (cmd.flags & ~IB_MR_REREG_SUPPORTED || !cmd.flags) return -EINVAL; @@ -881,7 +881,7 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); mw = pd->device->alloc_mw(pd, cmd.mw_type, &udata); if (IS_ERR(mw)) { @@ -1092,12 +1092,13 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file, return -EFAULT; ib_uverbs_init_udata(&ucore, buf, u64_to_user_ptr(cmd.response), - sizeof(cmd), sizeof(resp)); + sizeof(cmd), sizeof(resp), + ib_uverbs_get_ucontext(file)); ib_uverbs_init_udata(&uhw, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); memset(&cmd_ex, 0, sizeof(cmd_ex)); cmd_ex.user_handle = cmd.user_handle; @@ -1176,7 +1177,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); cq = uobj_get_obj_read(cq, UVERBS_OBJECT_CQ, cmd.cq_handle, file); if (!cq) @@ -1633,11 +1634,11 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, return -EFAULT; ib_uverbs_init_udata(&ucore, buf, u64_to_user_ptr(cmd.response), - sizeof(cmd), resp_size); + sizeof(cmd), resp_size, ib_uverbs_get_ucontext(file)); ib_uverbs_init_udata(&uhw, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + resp_size, in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - resp_size); + out_len - resp_size, ib_uverbs_get_ucontext(file)); memset(&cmd_ex, 0, sizeof(cmd_ex)); cmd_ex.user_handle = cmd.user_handle; @@ -1734,7 +1735,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); obj = (struct ib_uqp_object *)uobj_alloc(UVERBS_OBJECT_QP, file, &ib_dev); @@ -2086,7 +2087,7 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd.base), NULL, in_len - sizeof(cmd.base) - sizeof(struct ib_uverbs_cmd_hdr), - out_len); + out_len, ib_uverbs_get_ucontext(file)); ret = modify_qp(file, &cmd, &udata); if (ret) @@ -2570,7 +2571,7 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); uobj = uobj_alloc(UVERBS_OBJECT_AH, file, &ib_dev); if (IS_ERR(uobj)) @@ -3805,7 +3806,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); ret = __uverbs_create_xsrq(file, &xcmd, &udata); if (ret) @@ -3831,7 +3832,7 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file, ib_uverbs_init_udata(&udata, buf + sizeof(cmd), u64_to_user_ptr(cmd.response) + sizeof(resp), in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr), - out_len - sizeof(resp)); + out_len - sizeof(resp), ib_uverbs_get_ucontext(file)); ret = __uverbs_create_xsrq(file, &cmd, &udata); if (ret) @@ -3854,7 +3855,7 @@ ssize_t ib_uverbs_modify_srq(struct ib_uverbs_file *file, return -EFAULT; ib_uverbs_init_udata(&udata, buf + sizeof cmd, NULL, in_len - sizeof cmd, - out_len); + out_len, ib_uverbs_get_ucontext(file)); srq = uobj_get_obj_read(srq, UVERBS_OBJECT_SRQ, cmd.srq_handle, file); if (!srq) diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 12d8f8097574..a184e04cd123 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -775,13 +775,15 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, ib_uverbs_init_udata_buf_or_null(&ucore, buf, u64_to_user_ptr(ex_hdr.response), - hdr.in_words * 8, hdr.out_words * 8); + hdr.in_words * 8, hdr.out_words * 8, + ib_uverbs_get_ucontext(file)); ib_uverbs_init_udata_buf_or_null(&uhw, buf + ucore.inlen, u64_to_user_ptr(ex_hdr.response) + ucore.outlen, ex_hdr.provider_in_words * 8, - ex_hdr.provider_out_words * 8); + ex_hdr.provider_out_words * 8, + ib_uverbs_get_ucontext(file)); ret = uverbs_ex_cmd_table[command](file, &ucore, &uhw); ret = (ret) ? : count;
initialize the context field in ib_udata created from uverbs commands and extended commands path. Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> --- drivers/infiniband/core/uverbs.h | 9 ++++--- drivers/infiniband/core/uverbs_cmd.c | 35 ++++++++++++++------------- drivers/infiniband/core/uverbs_main.c | 6 +++-- 3 files changed, 28 insertions(+), 22 deletions(-)