Re: [PATCH 04/24] dma-buf: add dma_resv_get_singleton v2

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 03, 2022 at 12:13:41PM +0100, Christian König wrote:
> Am 22.12.21 um 22:21 schrieb Daniel Vetter:
> > On Tue, Dec 07, 2021 at 01:33:51PM +0100, Christian König wrote:
> > > Add a function to simplify getting a single fence for all the fences in
> > > the dma_resv object.
> > > 
> > > v2: fix ref leak in error handling
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/dma-buf/dma-resv.c | 52 ++++++++++++++++++++++++++++++++++++++
> > >   include/linux/dma-resv.h   |  2 ++
> > >   2 files changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> > > index 480c305554a1..694716a3d66d 100644
> > > --- a/drivers/dma-buf/dma-resv.c
> > > +++ b/drivers/dma-buf/dma-resv.c
> > > @@ -34,6 +34,7 @@
> > >    */
> > >   #include <linux/dma-resv.h>
> > > +#include <linux/dma-fence-array.h>
> > >   #include <linux/export.h>
> > >   #include <linux/mm.h>
> > >   #include <linux/sched/mm.h>
> > > @@ -657,6 +658,57 @@ int dma_resv_get_fences(struct dma_resv *obj, bool write,
> > >   }
> > >   EXPORT_SYMBOL_GPL(dma_resv_get_fences);
> > > +/**
> > > + * dma_resv_get_singleton - Get a single fence for all the fences
> > > + * @obj: the reservation object
> > > + * @write: true if we should return all fences
> > > + * @fence: the resulting fence
> > > + *
> > > + * Get a single fence representing all the fences inside the resv object.
> > > + * Returns either 0 for success or -ENOMEM.
> > > + *
> > > + * Warning: This can't be used like this when adding the fence back to the resv
> > > + * object since that can lead to stack corruption when finalizing the
> > > + * dma_fence_array.
> > Uh I don't get this one? I thought the only problem with nested fences is
> > the signalling recursion, which we work around with the irq_work?
> 
> Nope, the main problem is finalizing the dma_fence_array.
> 
> E.g. imagine that you build up a chain of dma_fence_array objects like this:
> a<-b<-c<-d<-e<-f.....
> 
> With each one referencing the previous dma_fence_array and then you call
> dma_fence_put() on the last one. That in turn will cause calling
> dma_fence_put() on the previous one, which in turn will cause
> dma_fence_put() one the one before the previous one etc....
> 
> In other words you recurse because each dma_fence_array instance drops the
> last reference of it's predecessor.
> 
> What we could do is to delegate dropping the reference to the containing
> fences in a dma_fence_array as well, but that would require some changes to
> the irq_work_run_list() function to be halve way efficient.o
> 
> > Also if there's really an issue with dma_fence_array fences, then that
> > warning should be on the dma_resv kerneldoc, not somewhere hidden like
> > this. And finally I really don't see what can go wrong, sure we'll end up
> > with the same fence once in the dma_resv_list and then once more in the
> > fence array. But they're all refcounted, so really shouldn't matter.
> > 
> > The code itself looks correct, but me not understanding what even goes
> > wrong here freaks me out a bit.
> 
> Yeah, IIRC we already discussed that with Jason in length as well.
> 
> Essentially what you can't do is to put a dma_fence_array into another
> dma_fence_array without causing issues.
> 
> So I think we should maybe just add a WARN_ON() into dma_fence_array_init()
> to make sure that this never happens.

Yeah I think this would be much clearer instead of sprinkling half the
story as a scary&confusing warning over all kinds of users which
internally use dma fence arrays.

And then if it goes boom I guess we could fix it internally in
dma_fence_array_init by flattening fences down again. But only if actually
needed.

What confused me is why dma_resv is special, and from your reply it sounds
like it really isn't.
-Daniel


> 
> Regards,
> Christian.
> 
> > 
> > I guess something to figure out next year, I kinda hoped I could squeeze a
> > review in before I disappear :-/
> > -Daniel
> > 
> > > + */
> > > +int dma_resv_get_singleton(struct dma_resv *obj, bool write,
> > > +			   struct dma_fence **fence)
> > > +{
> > > +	struct dma_fence_array *array;
> > > +	struct dma_fence **fences;
> > > +	unsigned count;
> > > +	int r;
> > > +
> > > +	r = dma_resv_get_fences(obj, write, &count, &fences);
> > > +        if (r)
> > > +		return r;
> > > +
> > > +	if (count == 0) {
> > > +		*fence = NULL;
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (count == 1) {
> > > +		*fence = fences[0];
> > > +		kfree(fences);
> > > +		return 0;
> > > +	}
> > > +
> > > +	array = dma_fence_array_create(count, fences,
> > > +				       dma_fence_context_alloc(1),
> > > +				       1, false);
> > > +	if (!array) {
> > > +		while (count--)
> > > +			dma_fence_put(fences[count]);
> > > +		kfree(fences);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	*fence = &array->base;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dma_resv_get_singleton);
> > > +
> > >   /**
> > >    * dma_resv_wait_timeout - Wait on reservation's objects
> > >    * shared and/or exclusive fences.
> > > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> > > index fa2002939b19..cdfbbda6f600 100644
> > > --- a/include/linux/dma-resv.h
> > > +++ b/include/linux/dma-resv.h
> > > @@ -438,6 +438,8 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
> > >   void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence);
> > >   int dma_resv_get_fences(struct dma_resv *obj, bool write,
> > >   			unsigned int *num_fences, struct dma_fence ***fences);
> > > +int dma_resv_get_singleton(struct dma_resv *obj, bool write,
> > > +			   struct dma_fence **fence);
> > >   int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
> > >   long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr,
> > >   			   unsigned long timeout);
> > > -- 
> > > 2.25.1
> > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch