On Tue, Jan 22, 2019 at 09:02:18AM -0700, Jason Gunthorpe wrote:
> 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?
Create path can have same issue... If driver object hold information on
the context and we want to share it to another context we have problem
that we cannot duplicate per context information (we might have more
then 2 context at that point so who is the up-to-date..?)
So in general I think:
1. Any ib_x driver object that use ib_ucontext to store data is not
shareable
2. The main thing object sharing need is that no ib_x object will
have ib_uobject pointer
I think that we need to do for the destroy APIs the same change as
you requested for the create APIs where we eliminate the dependency of
the core in the ib_uobject embedded in the ib_x objects (mainly to
obtain ib_ucontext pointer).
Will it be acceptable to eliminate the use of ib_uobject in destroy APIs
via the use of ib_udata embedded in attr bundle as next step?
>
> > 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
I think we need some way the rdma & uverbs could query the driver and
ask if specific object type can be shared or not within that driver.
Sound like that each driver need to supply this:
bool is_shareable(enum uverbs_default_objects object)
At same time I think that all the interface between uverbs - rdma -
driver need to support sharing. The meaning is that we need a way to
duplicate existing driver object and return the driver specific
information to user. For this to happen we need the ability to send the
original driver object down to the driver so it will find what it need
to create the second copy.
Is it acceptable to modify the rdma - driver interface for every create
API so it will have the missing information?
>
> 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.
Probably if driver want to enable share of some object it first need to
move all the related data of this object out of the ib_ucontext and then
add the code that duplicate the object and only then enable the sharing
via 'is_shareable'.
>
> > 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
