On Mon, Jan 17, 2022 at 5:26 AM Christian König <
ckoenig.leichtzumer...@gmail.com> wrote:
> Am 14.01.22 um 17:31 schrieb Daniel Vetter:
> > 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.koe...@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.
>
Agreed. WARN_ON in dma_fence_array_init() would be better for everyone, I
think.
> > 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.
>
> Ok, going to do that first then.
>
Sounds good. This patch looks pretty reasonable to me. I do have a bit of
a concern with how it's being used to replace calls to
dma_resv_excl_fence() in later patches, though. In particular, this may
allocate memory whereas dma_resv_excl_fence() does not so we need to be
really careful in each of the replacements that doing so is safe. That's a
job for the per-driver reviewers but I thought I'd drop a note here so
we're all aware of and watching for it.
--Jason
> >
> > What confused me is why dma_resv is special, and from your reply it
> sounds
> > like it really isn't.
>
> Well, it isn't special in any way. It's just something very obvious
> which could go wrong.
>
> Regards,
> Christian.
>
> > -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
> >>>>
>
>
