On Thu, Jan 17, 2019 at 01:38:20PM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 17, 2019 at 09:11:12PM +0200, Shamir Rabinovitch wrote:
> > Now when we have the udata passed to all the ib_xxx object creation APIs
> > and the additional function 'rdma_get_ucontext' to get the ib_ucontext
> > from ib_udata stored in uverbs_attr_bundle, we can finally start to remove
> > the dependency of the drivers in the ib_xxx->uobject->context.
> >
[...]
>
> Hurm.. I think if you are going to use this API so widely then we need
> to arrange for it to not fail. If udata is not null, and we are in a
> driver callback with an object, then the ucontext should be guaranteed
> available.
>
> Otherwise we have weird cases where udata and !rdma_get_ucontext() can
> be true, which is really horrible to reason about..
>
> The way to do this is to have the core code places that have a uobject
> copy the ucontext from the uobject into the udata, I think I mentioned
> this before, and it is a fixme in the code..
>
> For instance something like this would take care of things for the
> ioctl path:
>
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index 8c81ff69805276..9e9240645fbe9b 100644
> --- a/drivers/infiniband/core/uverbs_ioctl.c
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -198,6 +198,7 @@ static int uverbs_process_idrs_array(struct bundle_priv *pbundle,
> ret = PTR_ERR(attr->uobjects[i]);
> break;
> }
> + pbundle->bundle.ucontext = attr->ubojects[i].ucontext;
> }
>
> attr->len = i;
> @@ -316,6 +317,7 @@ static int uverbs_process_attr(struct bundle_priv *pbundle,
> if (IS_ERR(o_attr->uobject))
> return PTR_ERR(o_attr->uobject);
> __set_bit(attr_bkey, pbundle->uobj_finalize);
> + pbundle->bundle.ucontext = o_attr->uoboject.ucontext;
>
> if (spec->u.obj.access == UVERBS_ACCESS_NEW) {
> unsigned int uattr_idx = uattr - pbundle->uattrs;
>
> And the cmd path needs some editing of its alloc/get macros
OK. If I take ib_uverbs_alloc_pd as example I see:
ib_uverbs_write -> ib_uverbs_alloc_pd -> uobj_alloc -> __uobj_alloc ->
rdma_alloc_begin_uobject -> alloc_begin_idr_uobject -> alloc_uobj ->
ib_uverbs_get_ucontext_file
So it seems that the best place to pass the ib_ucontext to
uverbs_attr_bundle is in __uobj_alloc in this call chain.
I have this question about ib_ucontext in ib_uobject:
I see this comment above ib_uverbs_get_ucontext_file:
"
* Must be called with the ufile->device->disassociate_srcu held, and the lock
* must be held until use of the ucontext is finished.
"
Surly, when ib_uobject has pointer to ib_ucontext the above lock is not held for all
the duration.
What prevent the use of the ib_ucontext from ib_uobject after device has been
disassociated ?
Is it the fact that ib_uobject are only accessed via ib_uverbs_write for commands
which hold the disassociate_srcu and that uverbs_disassociate_api_pre disable the
commands for uverbs_api for that ib_uverbs_device ?
Are there any paths that touch ib_uobject but not with in ib_uverbs_write protection ?
Is this related to the point that you do not want to assume ib_udata (&context) in the
ib_xxx destroy path ?
If so, will it be acceptable to move the ib_xxx driver related data hanged on the ib_ucontext
to the ib_xxx driver object and prevent sharing of this specific object in this specific
driver to remove the dependency in the destroy path in the ib_ucontext ?
Will it be acceptable to have each driver to decide what objects are shareable and what
are not (those with data hanged on ib_ucontext are not shareable)?
> > + context = rdma_get_ucontext(udata);
> > + if (IS_ERR(context))
> > + return PTR_ERR(context);
>
> Like here, this is getting really long..
>
> And this should probably follow the pattern of the new
> rdma_device_to_drv_device() to make it simpler..
>
> Just add something like this at the start of functions:
>
> struct bnxt_re_ucontext *cntx = rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
>
> returns NULL if kernel mode.
>
> And then use 'if (cntx)' instead of 'if (udata)' in that function to
> test for user/kernel mode..
>
> Jason
OK.
