On Mon, Jan 28, 2019 at 12:20:42PM -0700, Jason Gunthorpe wrote:
> 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.
I think I was miss understood here... Duplicating the object is not
duplicating the ib_x HW object but rather letting the driver to copy the
related information from the existing ib_x object to the user-space
library. Example is the pdn number in mlx4 ib_bd creation.
For this to happen, the create API (alloc_pd) need to convey existing
ib_pd down to the mlx4 driver in case of share of PD.
I think it's best to modify all the APIs between rdma-core and drivers so
that it will be possible to attach existing ib_x object to each creation
API and so we will have support for sharing in this interface even if we
decide not to implement it because no driver currently support it.
This involve wide API change which I think it's best to do in single
commit.
>
> > 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
