On Tue, Jan 22, 2019 at 05:43:42PM +0200, Shamir Rabinovitch wrote:
> > @@ -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.
Probably yes
> 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.
The comment is only for ib_uverbs_get_ucontext_file() callers.
But for the ucontext in the ib_uobject - all accesses to it have to be
covered under a srcu & UVERBS_LOOKUP_READ/WRITE or the
hw_destroy_rwsem to make sure the pointer is still
alive. uverbs_destroy_uobject() sets the ucontext to NULL during
disassociate, so every reader must have locking against that set.
> 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 ?
Running under the commands is what provides the srcu lock, but this
test here in rdma_lookup_get_uobject:
/*
* If we have been disassociated block every command except for
* DESTROY based commands.
*/
if (mode != UVERBS_LOOKUP_DESTROY &&
!srcu_dereference(ufile->device->ib_dev,
&ufile->device->disassociate_srcu)) {
ret = uverbs_try_lock_object(uobj, mode);
Is what is excluding parallel dis-association. Once the per-uobject
read/write lock is obtained under the srcu the ucontext pointer cannot be
null'd.
> Are there any paths that touch ib_uobject but not with in
> ib_uverbs_write protection ?
Everything should use this system.
> Is this related to the point that you do not want to assume ib_udata
> (&context) in the ib_xxx destroy path ?
Not really.. the destroy path will still have a ucontext since
ucontext cannot be destroyed until all of the ubojects are destroyed,
but under sharing it might not be the same ucontext that was used to
create the object - so what use could destroy have of it?
> 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 ?
I don't know.. We'd see what that looks like
Maybe the driver should retain a kref on the ucontext inside the
objects that need it, but that would be a bit tricky to manage
lifetime too.
> 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)?
I think that is what we will end up with, as revising all the drivers
does not seem feasible.
Jason
