| Message ID | 20211207123411.167006-5-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/24] dma-buf: add dma_resv_replace_fences | expand |
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? 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. 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 >
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. > 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. 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 >>
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. What confused me is why dma_resv is special, and from your reply it sounds like it really isn't. -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 > > > >
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 >>>>
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); > >>>> 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(+)