On Mon, Jan 17, 2022 at 5:26 AM Christian König < ckoenig.leichtzumerken@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.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.
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);*src);
long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all,
bool intr,
unsigned long timeout);-- 2.25.1