On Mon, Jan 28, 2019 at 07:03:47PM +0200, Shamir Rabinovitch wrote:
> 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..?)
Well, a driver might be able to get by if it was syemmtric (ie always release
the resource against the allocating PD) but it complicates ucontext
destruction.
> 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?
Yes, we can do that.. For non-sharable objects it would be no change
in behavior.
> 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)
I think drivers will need to have a flags that says what objects they
can allow for sharing. Each driver will have to be audited before they
can support sharing unfortunately.
> 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.
I don't think we should be duplicating the driver object, sharing
should be a uverbs thing where it just creates a new uobject and
points to the same driver object. Somehow this has to work out
a 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'.
Most likely
Jason
