<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jan 17, 2022 at 5:26 AM Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>> >>>> --- >>>> 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. </blockquote><div> </div><div>Agreed. WARN_ON in dma_fence_array_init() would be better for everyone, I think. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > 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. </blockquote><div> </div><div>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.</div><div> </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > 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 >>>> </blockquote></div></div>