On Mon, Jan 17, 2022 at 5:26 AM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> 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.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.
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
>>>>
