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@xxxxxxx>
> > > ---
> > > 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
