Re: [PATCH 08/25] IB/uverbs: ufile must be freed only when not used anymore

[Shamir Rabinovitch <srabinov7@gmail.com>]

On Wed, Jul 17, 2019 at 08:53:54AM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 16, 2019 at 09:11:43PM +0300, Shamir Rabinovitch wrote:
> > From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> > 
> > ufile (&ucontext) with the process who own them must not be released
> > when there are other ufile (&ucontext) that depens at them.
> 
> We already have a kref, why do we need more? Especially wrongly done
> refcounts with atomics?

Yes. Will fix in v2.

> 
> Trying to sequence the destroy of the ucontext seems inherently wrong
> to me. If the driver has to link the PD/MR to data in the ucontext it
> can't support sharing.

The issue we try to solve here is this:

[process 1]                     [process 2]
- alloc mr & point mr to        -
  context 1                     
- share context                 -
-                               - import mr
- exit                          -
-- ufile_destroy_ucontext       -
--- ib_mr is not destroyed      -
--- context 1 is destroyed      -
-                               - exit
-                               -- ufile_destroy_ucontext
-                               --- driver dereg_mr is called
-                               ---- ib_umem_release on umem from
                                     previously destroyed context 1

If I recall correctly, you suggested the shere and shree concept.

We also talked with Mellanox architecture team and they suggested
that the shrere will be bullet proof process that *only* create and
share objects.

The whole thing directly links to the next step we talked about 
which is sharing objects via file system rathen then via FD.

> 
> > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> > Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com>
> >  drivers/infiniband/core/rdma_core.c   | 29 +++++++++++++++++++++++++++
> >  drivers/infiniband/core/uverbs.h      | 22 ++++++++++++++++++++
> >  drivers/infiniband/core/uverbs_cmd.c  | 16 +++++++++++++++
> >  drivers/infiniband/core/uverbs_main.c |  4 ++++
> >  4 files changed, 71 insertions(+)
> > 
> > diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> > index 651625f632d7..c81ff8e28fc6 100644
> > +++ b/drivers/infiniband/core/rdma_core.c
> > @@ -841,6 +841,33 @@ static void ufile_destroy_ucontext(struct ib_uverbs_file *ufile,
> >  	ufile->ucontext = NULL;
> >  }
> >  
> > +static void __uverbs_ufile_refcount(struct ib_uverbs_file *ufile)
> > +{
> > +	int wait;
> > +
> > +	if (ufile->parent) {
> > +		pr_debug("%s: release parent ufile. ufile %p parent %p\n",
> > +			 __func__, ufile, ufile->parent);
> > +		if (atomic_dec_and_test(&ufile->parent->refcount))
> > +			complete(&ufile->parent->context_released);
> > +	}
> > +
> > +	if (!atomic_dec_and_test(&ufile->refcount)) {
> > +wait:
> > +		wait = wait_for_completion_interruptible_timeout(
> > +			&ufile->context_released, 3*HZ);
> > +		if (wait == -ERESTARTSYS) {
> > +			WARN_ONCE(1,
> > +			"signal while waiting for context release! ufile %p\n",
> > +				ufile);
> 
> ????
> 
> Jason

I copied the behaviour I saw in the rest of the kernel as for what to do
when wait_for_completion_interruptible_timeout exit due to interrupt.

From the above reason I think we need to delay the shrere process exit
so it will not close the context prematurely *unless* it receive signal.

In that case I'd expect that process to soot some clear warning and do 
whatevere id need to do for the given signal.