| Message ID | 20211207123411.167006-5-christian.koenig@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/24] dma-buf: add dma_resv_replace_fences | expand |
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@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. Ok, going to do that first then. > > 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 >>>>
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. + */ +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);
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(+)