| Message ID | 20181004131250.2373-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] dma-buf: remove shared fence staging in reservation object | expand |
Ping! Adding a few people directly and more mailing lists. Can I get an acked-by/rb for this? It's only a cleanup and not much functional change. Regards, Christian. Am 04.10.2018 um 15:12 schrieb Christian König: > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- > include/linux/reservation.h | 4 - > 2 files changed, 58 insertions(+), 124 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 6c95f61a32e7..5825fc336a13 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > */ > int reservation_object_reserve_shared(struct reservation_object *obj) > { > - struct reservation_object_list *fobj, *old; > - u32 max; > + struct reservation_object_list *old, *new; > + unsigned int i, j, k, max; > > old = reservation_object_get_list(obj); > > if (old && old->shared_max) { > - if (old->shared_count < old->shared_max) { > - /* perform an in-place update */ > - kfree(obj->staged); > - obj->staged = NULL; > + if (old->shared_count < old->shared_max) > return 0; > - } else > + else > max = old->shared_max * 2; > - } else > - max = 4; > - > - /* > - * resize obj->staged or allocate if it doesn't exist, > - * noop if already correct size > - */ > - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), > - GFP_KERNEL); > - if (!fobj) > - return -ENOMEM; > - > - obj->staged = fobj; > - fobj->shared_max = max; > - return 0; > -} > -EXPORT_SYMBOL(reservation_object_reserve_shared); > - > -static void > -reservation_object_add_shared_inplace(struct reservation_object *obj, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - struct dma_fence *signaled = NULL; > - u32 i, signaled_idx; > - > - dma_fence_get(fence); > - > - preempt_disable(); > - write_seqcount_begin(&obj->seq); > - > - for (i = 0; i < fobj->shared_count; ++i) { > - struct dma_fence *old_fence; > - > - old_fence = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - > - if (old_fence->context == fence->context) { > - /* memory barrier is added by write_seqcount_begin */ > - RCU_INIT_POINTER(fobj->shared[i], fence); > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(old_fence); > - return; > - } > - > - if (!signaled && dma_fence_is_signaled(old_fence)) { > - signaled = old_fence; > - signaled_idx = i; > - } > - } > - > - /* > - * memory barrier is added by write_seqcount_begin, > - * fobj->shared_count is protected by this lock too > - */ > - if (signaled) { > - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > - BUG_ON(fobj->shared_count >= fobj->shared_max); > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + max = 4; > } > > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(signaled); > -} > - > -static void > -reservation_object_add_shared_replace(struct reservation_object *obj, > - struct reservation_object_list *old, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - unsigned i, j, k; > - > - dma_fence_get(fence); > - > - if (!old) { > - RCU_INIT_POINTER(fobj->shared[0], fence); > - fobj->shared_count = 1; > - goto done; > - } > + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > + if (!new) > + return -ENOMEM; > > /* > * no need to bump fence refcounts, rcu_read access > @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > * references from the old struct are carried over to > * the new. > */ > - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { > - struct dma_fence *check; > + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { > + struct dma_fence *fence; > > - check = rcu_dereference_protected(old->shared[i], > - reservation_object_held(obj)); > - > - if (check->context == fence->context || > - dma_fence_is_signaled(check)) > - RCU_INIT_POINTER(fobj->shared[--k], check); > + fence = rcu_dereference_protected(old->shared[i], > + reservation_object_held(obj)); > + if (dma_fence_is_signaled(fence)) > + RCU_INIT_POINTER(new->shared[--k], fence); > else > - RCU_INIT_POINTER(fobj->shared[j++], check); > + RCU_INIT_POINTER(new->shared[j++], fence); > } > - fobj->shared_count = j; > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + new->shared_count = j; > + new->shared_max = max; > > -done: > preempt_disable(); > write_seqcount_begin(&obj->seq); > /* > * RCU_INIT_POINTER can be used here, > * seqcount provides the necessary barriers > */ > - RCU_INIT_POINTER(obj->fence, fobj); > + RCU_INIT_POINTER(obj->fence, new); > write_seqcount_end(&obj->seq); > preempt_enable(); > > if (!old) > - return; > + return 0; > > /* Drop the references to the signaled fences */ > - for (i = k; i < fobj->shared_max; ++i) { > - struct dma_fence *f; > + for (i = k; i < new->shared_max; ++i) { > + struct dma_fence *fence; > > - f = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - dma_fence_put(f); > + fence = rcu_dereference_protected(new->shared[i], > + reservation_object_held(obj)); > + dma_fence_put(fence); > } > kfree_rcu(old, rcu); > + > + return 0; > } > +EXPORT_SYMBOL(reservation_object_reserve_shared); > > /** > * reservation_object_add_shared_fence - Add a fence to a shared slot > @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > void reservation_object_add_shared_fence(struct reservation_object *obj, > struct dma_fence *fence) > { > - struct reservation_object_list *old, *fobj = obj->staged; > + struct reservation_object_list *fobj; > + unsigned int i; > > - old = reservation_object_get_list(obj); > - obj->staged = NULL; > + dma_fence_get(fence); > + > + fobj = reservation_object_get_list(obj); > > - if (!fobj) > - reservation_object_add_shared_inplace(obj, old, fence); > - else > - reservation_object_add_shared_replace(obj, old, fobj, fence); > + preempt_disable(); > + write_seqcount_begin(&obj->seq); > + > + for (i = 0; i < fobj->shared_count; ++i) { > + struct dma_fence *old_fence; > + > + old_fence = rcu_dereference_protected(fobj->shared[i], > + reservation_object_held(obj)); > + if (old_fence->context == fence->context || > + dma_fence_is_signaled(old_fence)) { > + dma_fence_put(old_fence); > + goto replace; > + } > + } > + > + BUG_ON(fobj->shared_count >= fobj->shared_max); > + fobj->shared_count++; > + > +replace: > + /* > + * memory barrier is added by write_seqcount_begin, > + * fobj->shared_count is protected by this lock too > + */ > + RCU_INIT_POINTER(fobj->shared[i], fence); > + write_seqcount_end(&obj->seq); > + preempt_enable(); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence); > > @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, > new = dma_fence_get_rcu_safe(&src->fence_excl); > rcu_read_unlock(); > > - kfree(dst->staged); > - dst->staged = NULL; > - > src_list = reservation_object_get_list(dst); > old = reservation_object_get_excl(dst); > > diff --git a/include/linux/reservation.h b/include/linux/reservation.h > index 02166e815afb..54cf6773a14c 100644 > --- a/include/linux/reservation.h > +++ b/include/linux/reservation.h > @@ -68,7 +68,6 @@ struct reservation_object_list { > * @seq: sequence count for managing RCU read-side synchronization > * @fence_excl: the exclusive fence, if there is one currently > * @fence: list of current shared fences > - * @staged: staged copy of shared fences for RCU updates > */ > struct reservation_object { > struct ww_mutex lock; > @@ -76,7 +75,6 @@ struct reservation_object { > > struct dma_fence __rcu *fence_excl; > struct reservation_object_list __rcu *fence; > - struct reservation_object_list *staged; > }; > > #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) > @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) > __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > - obj->staged = NULL; > } > > /** > @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) > > kfree(fobj); > } > - kfree(obj->staged); > > ww_mutex_destroy(&obj->lock); > }
Ping once more! Adding a few more AMD people. Any comments on this? Thanks, Christian. Am 12.10.18 um 10:22 schrieb Christian König: > Ping! Adding a few people directly and more mailing lists. > > Can I get an acked-by/rb for this? It's only a cleanup and not much > functional change. > > Regards, > Christian. > > Am 04.10.2018 um 15:12 schrieb Christian König: >> No need for that any more. Just replace the list when there isn't enough >> room any more for the additional fence. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/reservation.c | 178 >> ++++++++++++++---------------------------- >> include/linux/reservation.h | 4 - >> 2 files changed, 58 insertions(+), 124 deletions(-) >> >> diff --git a/drivers/dma-buf/reservation.c >> b/drivers/dma-buf/reservation.c >> index 6c95f61a32e7..5825fc336a13 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); >> */ >> int reservation_object_reserve_shared(struct reservation_object *obj) >> { >> - struct reservation_object_list *fobj, *old; >> - u32 max; >> + struct reservation_object_list *old, *new; >> + unsigned int i, j, k, max; >> old = reservation_object_get_list(obj); >> if (old && old->shared_max) { >> - if (old->shared_count < old->shared_max) { >> - /* perform an in-place update */ >> - kfree(obj->staged); >> - obj->staged = NULL; >> + if (old->shared_count < old->shared_max) >> return 0; >> - } else >> + else >> max = old->shared_max * 2; >> - } else >> - max = 4; >> - >> - /* >> - * resize obj->staged or allocate if it doesn't exist, >> - * noop if already correct size >> - */ >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), >> - GFP_KERNEL); >> - if (!fobj) >> - return -ENOMEM; >> - >> - obj->staged = fobj; >> - fobj->shared_max = max; >> - return 0; >> -} >> -EXPORT_SYMBOL(reservation_object_reserve_shared); >> - >> -static void >> -reservation_object_add_shared_inplace(struct reservation_object *obj, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - struct dma_fence *signaled = NULL; >> - u32 i, signaled_idx; >> - >> - dma_fence_get(fence); >> - >> - preempt_disable(); >> - write_seqcount_begin(&obj->seq); >> - >> - for (i = 0; i < fobj->shared_count; ++i) { >> - struct dma_fence *old_fence; >> - >> - old_fence = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - >> - if (old_fence->context == fence->context) { >> - /* memory barrier is added by write_seqcount_begin */ >> - RCU_INIT_POINTER(fobj->shared[i], fence); >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(old_fence); >> - return; >> - } >> - >> - if (!signaled && dma_fence_is_signaled(old_fence)) { >> - signaled = old_fence; >> - signaled_idx = i; >> - } >> - } >> - >> - /* >> - * memory barrier is added by write_seqcount_begin, >> - * fobj->shared_count is protected by this lock too >> - */ >> - if (signaled) { >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); >> } else { >> - BUG_ON(fobj->shared_count >= fobj->shared_max); >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + max = 4; >> } >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(signaled); >> -} >> - >> -static void >> -reservation_object_add_shared_replace(struct reservation_object *obj, >> - struct reservation_object_list *old, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - unsigned i, j, k; >> - >> - dma_fence_get(fence); >> - >> - if (!old) { >> - RCU_INIT_POINTER(fobj->shared[0], fence); >> - fobj->shared_count = 1; >> - goto done; >> - } >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); >> + if (!new) >> + return -ENOMEM; >> /* >> * no need to bump fence refcounts, rcu_read access >> @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> * references from the old struct are carried over to >> * the new. >> */ >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; >> ++i) { >> - struct dma_fence *check; >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); >> ++i) { >> + struct dma_fence *fence; >> - check = rcu_dereference_protected(old->shared[i], >> - reservation_object_held(obj)); >> - >> - if (check->context == fence->context || >> - dma_fence_is_signaled(check)) >> - RCU_INIT_POINTER(fobj->shared[--k], check); >> + fence = rcu_dereference_protected(old->shared[i], >> + reservation_object_held(obj)); >> + if (dma_fence_is_signaled(fence)) >> + RCU_INIT_POINTER(new->shared[--k], fence); >> else >> - RCU_INIT_POINTER(fobj->shared[j++], check); >> + RCU_INIT_POINTER(new->shared[j++], fence); >> } >> - fobj->shared_count = j; >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + new->shared_count = j; >> + new->shared_max = max; >> -done: >> preempt_disable(); >> write_seqcount_begin(&obj->seq); >> /* >> * RCU_INIT_POINTER can be used here, >> * seqcount provides the necessary barriers >> */ >> - RCU_INIT_POINTER(obj->fence, fobj); >> + RCU_INIT_POINTER(obj->fence, new); >> write_seqcount_end(&obj->seq); >> preempt_enable(); >> if (!old) >> - return; >> + return 0; >> /* Drop the references to the signaled fences */ >> - for (i = k; i < fobj->shared_max; ++i) { >> - struct dma_fence *f; >> + for (i = k; i < new->shared_max; ++i) { >> + struct dma_fence *fence; >> - f = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - dma_fence_put(f); >> + fence = rcu_dereference_protected(new->shared[i], >> + reservation_object_held(obj)); >> + dma_fence_put(fence); >> } >> kfree_rcu(old, rcu); >> + >> + return 0; >> } >> +EXPORT_SYMBOL(reservation_object_reserve_shared); >> /** >> * reservation_object_add_shared_fence - Add a fence to a shared slot >> @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> void reservation_object_add_shared_fence(struct reservation_object >> *obj, >> struct dma_fence *fence) >> { >> - struct reservation_object_list *old, *fobj = obj->staged; >> + struct reservation_object_list *fobj; >> + unsigned int i; >> - old = reservation_object_get_list(obj); >> - obj->staged = NULL; >> + dma_fence_get(fence); >> + >> + fobj = reservation_object_get_list(obj); >> - if (!fobj) >> - reservation_object_add_shared_inplace(obj, old, fence); >> - else >> - reservation_object_add_shared_replace(obj, old, fobj, fence); >> + preempt_disable(); >> + write_seqcount_begin(&obj->seq); >> + >> + for (i = 0; i < fobj->shared_count; ++i) { >> + struct dma_fence *old_fence; >> + >> + old_fence = rcu_dereference_protected(fobj->shared[i], >> + reservation_object_held(obj)); >> + if (old_fence->context == fence->context || >> + dma_fence_is_signaled(old_fence)) { >> + dma_fence_put(old_fence); >> + goto replace; >> + } >> + } >> + >> + BUG_ON(fobj->shared_count >= fobj->shared_max); >> + fobj->shared_count++; >> + >> +replace: >> + /* >> + * memory barrier is added by write_seqcount_begin, >> + * fobj->shared_count is protected by this lock too >> + */ >> + RCU_INIT_POINTER(fobj->shared[i], fence); >> + write_seqcount_end(&obj->seq); >> + preempt_enable(); >> } >> EXPORT_SYMBOL(reservation_object_add_shared_fence); >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct >> reservation_object *dst, >> new = dma_fence_get_rcu_safe(&src->fence_excl); >> rcu_read_unlock(); >> - kfree(dst->staged); >> - dst->staged = NULL; >> - >> src_list = reservation_object_get_list(dst); >> old = reservation_object_get_excl(dst); >> diff --git a/include/linux/reservation.h b/include/linux/reservation.h >> index 02166e815afb..54cf6773a14c 100644 >> --- a/include/linux/reservation.h >> +++ b/include/linux/reservation.h >> @@ -68,7 +68,6 @@ struct reservation_object_list { >> * @seq: sequence count for managing RCU read-side synchronization >> * @fence_excl: the exclusive fence, if there is one currently >> * @fence: list of current shared fences >> - * @staged: staged copy of shared fences for RCU updates >> */ >> struct reservation_object { >> struct ww_mutex lock; >> @@ -76,7 +75,6 @@ struct reservation_object { >> struct dma_fence __rcu *fence_excl; >> struct reservation_object_list __rcu *fence; >> - struct reservation_object_list *staged; >> }; >> #define reservation_object_held(obj) >> lockdep_is_held(&(obj)->lock.base) >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object >> *obj) >> __seqcount_init(&obj->seq, reservation_seqcount_string, >> &reservation_seqcount_class); >> RCU_INIT_POINTER(obj->fence, NULL); >> RCU_INIT_POINTER(obj->fence_excl, NULL); >> - obj->staged = NULL; >> } >> /** >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object >> *obj) >> kfree(fobj); >> } >> - kfree(obj->staged); >> ww_mutex_destroy(&obj->lock); >> } >
On 2018-10-23 2:20 p.m., Christian König wrote: > Ping once more! Adding a few more AMD people. > > Any comments on this? Patches 1 & 3 are a bit over my head I'm afraid. Patches 2, 4, 6-8 are Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Christian, I will take a look at them later. Thanks, Ray > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > >
Patch 3, 5 is Acked-by: Junwei Zhang <Jerry.Zhang@amd.com> Others are Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com> On 10/4/18 9:12 PM, Christian König wrote: > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- > include/linux/reservation.h | 4 - > 2 files changed, 58 insertions(+), 124 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 6c95f61a32e7..5825fc336a13 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > */ > int reservation_object_reserve_shared(struct reservation_object *obj) > { > - struct reservation_object_list *fobj, *old; > - u32 max; > + struct reservation_object_list *old, *new; > + unsigned int i, j, k, max; > > old = reservation_object_get_list(obj); > > if (old && old->shared_max) { > - if (old->shared_count < old->shared_max) { > - /* perform an in-place update */ > - kfree(obj->staged); > - obj->staged = NULL; > + if (old->shared_count < old->shared_max) > return 0; > - } else > + else > max = old->shared_max * 2; > - } else > - max = 4; > - > - /* > - * resize obj->staged or allocate if it doesn't exist, > - * noop if already correct size > - */ > - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), > - GFP_KERNEL); > - if (!fobj) > - return -ENOMEM; > - > - obj->staged = fobj; > - fobj->shared_max = max; > - return 0; > -} > -EXPORT_SYMBOL(reservation_object_reserve_shared); > - > -static void > -reservation_object_add_shared_inplace(struct reservation_object *obj, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - struct dma_fence *signaled = NULL; > - u32 i, signaled_idx; > - > - dma_fence_get(fence); > - > - preempt_disable(); > - write_seqcount_begin(&obj->seq); > - > - for (i = 0; i < fobj->shared_count; ++i) { > - struct dma_fence *old_fence; > - > - old_fence = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - > - if (old_fence->context == fence->context) { > - /* memory barrier is added by write_seqcount_begin */ > - RCU_INIT_POINTER(fobj->shared[i], fence); > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(old_fence); > - return; > - } > - > - if (!signaled && dma_fence_is_signaled(old_fence)) { > - signaled = old_fence; > - signaled_idx = i; > - } > - } > - > - /* > - * memory barrier is added by write_seqcount_begin, > - * fobj->shared_count is protected by this lock too > - */ > - if (signaled) { > - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > - BUG_ON(fobj->shared_count >= fobj->shared_max); > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + max = 4; > } > > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(signaled); > -} > - > -static void > -reservation_object_add_shared_replace(struct reservation_object *obj, > - struct reservation_object_list *old, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - unsigned i, j, k; > - > - dma_fence_get(fence); > - > - if (!old) { > - RCU_INIT_POINTER(fobj->shared[0], fence); > - fobj->shared_count = 1; > - goto done; > - } > + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > + if (!new) > + return -ENOMEM; > > /* > * no need to bump fence refcounts, rcu_read access > @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > * references from the old struct are carried over to > * the new. > */ > - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { > - struct dma_fence *check; > + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { > + struct dma_fence *fence; > > - check = rcu_dereference_protected(old->shared[i], > - reservation_object_held(obj)); > - > - if (check->context == fence->context || > - dma_fence_is_signaled(check)) > - RCU_INIT_POINTER(fobj->shared[--k], check); > + fence = rcu_dereference_protected(old->shared[i], > + reservation_object_held(obj)); > + if (dma_fence_is_signaled(fence)) > + RCU_INIT_POINTER(new->shared[--k], fence); > else > - RCU_INIT_POINTER(fobj->shared[j++], check); > + RCU_INIT_POINTER(new->shared[j++], fence); > } > - fobj->shared_count = j; > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + new->shared_count = j; > + new->shared_max = max; > > -done: > preempt_disable(); > write_seqcount_begin(&obj->seq); > /* > * RCU_INIT_POINTER can be used here, > * seqcount provides the necessary barriers > */ > - RCU_INIT_POINTER(obj->fence, fobj); > + RCU_INIT_POINTER(obj->fence, new); > write_seqcount_end(&obj->seq); > preempt_enable(); > > if (!old) > - return; > + return 0; > > /* Drop the references to the signaled fences */ > - for (i = k; i < fobj->shared_max; ++i) { > - struct dma_fence *f; > + for (i = k; i < new->shared_max; ++i) { > + struct dma_fence *fence; > > - f = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - dma_fence_put(f); > + fence = rcu_dereference_protected(new->shared[i], > + reservation_object_held(obj)); > + dma_fence_put(fence); > } > kfree_rcu(old, rcu); > + > + return 0; > } > +EXPORT_SYMBOL(reservation_object_reserve_shared); > > /** > * reservation_object_add_shared_fence - Add a fence to a shared slot > @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > void reservation_object_add_shared_fence(struct reservation_object *obj, > struct dma_fence *fence) > { > - struct reservation_object_list *old, *fobj = obj->staged; > + struct reservation_object_list *fobj; > + unsigned int i; > > - old = reservation_object_get_list(obj); > - obj->staged = NULL; > + dma_fence_get(fence); > + > + fobj = reservation_object_get_list(obj); > > - if (!fobj) > - reservation_object_add_shared_inplace(obj, old, fence); > - else > - reservation_object_add_shared_replace(obj, old, fobj, fence); > + preempt_disable(); > + write_seqcount_begin(&obj->seq); > + > + for (i = 0; i < fobj->shared_count; ++i) { > + struct dma_fence *old_fence; > + > + old_fence = rcu_dereference_protected(fobj->shared[i], > + reservation_object_held(obj)); > + if (old_fence->context == fence->context || > + dma_fence_is_signaled(old_fence)) { > + dma_fence_put(old_fence); > + goto replace; > + } > + } > + > + BUG_ON(fobj->shared_count >= fobj->shared_max); > + fobj->shared_count++; > + > +replace: > + /* > + * memory barrier is added by write_seqcount_begin, > + * fobj->shared_count is protected by this lock too > + */ > + RCU_INIT_POINTER(fobj->shared[i], fence); > + write_seqcount_end(&obj->seq); > + preempt_enable(); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence); > > @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, > new = dma_fence_get_rcu_safe(&src->fence_excl); > rcu_read_unlock(); > > - kfree(dst->staged); > - dst->staged = NULL; > - > src_list = reservation_object_get_list(dst); > old = reservation_object_get_excl(dst); > > diff --git a/include/linux/reservation.h b/include/linux/reservation.h > index 02166e815afb..54cf6773a14c 100644 > --- a/include/linux/reservation.h > +++ b/include/linux/reservation.h > @@ -68,7 +68,6 @@ struct reservation_object_list { > * @seq: sequence count for managing RCU read-side synchronization > * @fence_excl: the exclusive fence, if there is one currently > * @fence: list of current shared fences > - * @staged: staged copy of shared fences for RCU updates > */ > struct reservation_object { > struct ww_mutex lock; > @@ -76,7 +75,6 @@ struct reservation_object { > > struct dma_fence __rcu *fence_excl; > struct reservation_object_list __rcu *fence; > - struct reservation_object_list *staged; > }; > > #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) > @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) > __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > - obj->staged = NULL; > } > > /** > @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) > > kfree(fobj); > } > - kfree(obj->staged); > > ww_mutex_destroy(&obj->lock); > }
Series are Reviewed-by: Huang Rui <ray.huang@amd.com> > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > >
Quoting Christian König (2018-10-04 14:12:43) > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. Just a heads up. After this series landed, we started hitting a use-after-free when iterating the shared list. <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 -Chris
Quoting Chris Wilson (2018-10-25 21:15:17) > Quoting Christian König (2018-10-04 14:12:43) > > No need for that any more. Just replace the list when there isn't enough > > room any more for the additional fence. > > Just a heads up. After this series landed, we started hitting a > use-after-free when iterating the shared list. > > <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI > <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 > <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 > <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] > <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 > <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 > <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 > <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 > <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 > <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 > <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b > <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 > <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 First guess would be diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 5fb4fd461908..833698a0d548 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, } BUG_ON(fobj->shared_count >= fobj->shared_max); - fobj->shared_count++; replace: /* @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, * fobj->shared_count is protected by this lock too */ RCU_INIT_POINTER(fobj->shared[i], fence); + if (i == fobj->shared_count) + fobj->shared_count++; write_seqcount_end(&obj->seq); preempt_enable(); } Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? -Chris
Quoting Chris Wilson (2018-10-25 21:20:21) > Quoting Chris Wilson (2018-10-25 21:15:17) > > Quoting Christian König (2018-10-04 14:12:43) > > > No need for that any more. Just replace the list when there isn't enough > > > room any more for the additional fence. > > > > Just a heads up. After this series landed, we started hitting a > > use-after-free when iterating the shared list. > > > > <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI > > <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 > > <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 > > <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] > > <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 > > <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 > > <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 > > <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 > > <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 > > <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 > > <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b > > <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 > > <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 > > First guess would be > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 5fb4fd461908..833698a0d548 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, > } > > BUG_ON(fobj->shared_count >= fobj->shared_max); > - fobj->shared_count++; > > replace: > /* > @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, > * fobj->shared_count is protected by this lock too > */ > RCU_INIT_POINTER(fobj->shared[i], fence); > + if (i == fobj->shared_count) > + fobj->shared_count++; > write_seqcount_end(&obj->seq); > preempt_enable(); > } > > Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? Updating shared_count after setting the fence does the trick. -Chris
Am 25.10.18 um 23:21 schrieb Chris Wilson: > Quoting Chris Wilson (2018-10-25 21:20:21) >> Quoting Chris Wilson (2018-10-25 21:15:17) >>> Quoting Christian König (2018-10-04 14:12:43) >>>> No need for that any more. Just replace the list when there isn't enough >>>> room any more for the additional fence. >>> Just a heads up. After this series landed, we started hitting a >>> use-after-free when iterating the shared list. >>> >>> <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI >>> <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 >>> <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 >>> <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] >>> <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 >>> <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 >>> <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 >>> <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 >>> <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 >>> <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 >>> <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b >>> <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 >>> <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 >> First guess would be >> >> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >> index 5fb4fd461908..833698a0d548 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, >> } >> >> BUG_ON(fobj->shared_count >= fobj->shared_max); >> - fobj->shared_count++; >> >> replace: >> /* >> @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, >> * fobj->shared_count is protected by this lock too >> */ >> RCU_INIT_POINTER(fobj->shared[i], fence); >> + if (i == fobj->shared_count) >> + fobj->shared_count++; >> write_seqcount_end(&obj->seq); >> preempt_enable(); >> } >> >> Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? > Updating shared_count after setting the fence does the trick. Ah, crap. I can stare at the code for ages and don't see that problem. Neither did any internal testing showed any issues. Anyway your change is Reviewed-by: Christian König <christian.koenig@amd.com> Thanks for the quick fix, Christian. > -Chris
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); */ int reservation_object_reserve_shared(struct reservation_object *obj) { - struct reservation_object_list *fobj, *old; - u32 max; + struct reservation_object_list *old, *new; + unsigned int i, j, k, max; old = reservation_object_get_list(obj); if (old && old->shared_max) { - if (old->shared_count < old->shared_max) { - /* perform an in-place update */ - kfree(obj->staged); - obj->staged = NULL; + if (old->shared_count < old->shared_max) return 0; - } else + else max = old->shared_max * 2; - } else - max = 4; - - /* - * resize obj->staged or allocate if it doesn't exist, - * noop if already correct size - */ - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), - GFP_KERNEL); - if (!fobj) - return -ENOMEM; - - obj->staged = fobj; - fobj->shared_max = max; - return 0; -} -EXPORT_SYMBOL(reservation_object_reserve_shared); - -static void -reservation_object_add_shared_inplace(struct reservation_object *obj, - struct reservation_object_list *fobj, - struct dma_fence *fence) -{ - struct dma_fence *signaled = NULL; - u32 i, signaled_idx; - - dma_fence_get(fence); - - preempt_disable(); - write_seqcount_begin(&obj->seq); - - for (i = 0; i < fobj->shared_count; ++i) { - struct dma_fence *old_fence; - - old_fence = rcu_dereference_protected(fobj->shared[i], - reservation_object_held(obj)); - - if (old_fence->context == fence->context) { - /* memory barrier is added by write_seqcount_begin */ - RCU_INIT_POINTER(fobj->shared[i], fence); - write_seqcount_end(&obj->seq); - preempt_enable(); - - dma_fence_put(old_fence); - return; - } - - if (!signaled && dma_fence_is_signaled(old_fence)) { - signaled = old_fence; - signaled_idx = i; - } - } - - /* - * memory barrier is added by write_seqcount_begin, - * fobj->shared_count is protected by this lock too - */ - if (signaled) { - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); } else { - BUG_ON(fobj->shared_count >= fobj->shared_max); - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); - fobj->shared_count++; + max = 4; } - write_seqcount_end(&obj->seq); - preempt_enable(); - - dma_fence_put(signaled); -} - -static void -reservation_object_add_shared_replace(struct reservation_object *obj, - struct reservation_object_list *old, - struct reservation_object_list *fobj, - struct dma_fence *fence) -{ - unsigned i, j, k; - - dma_fence_get(fence); - - if (!old) { - RCU_INIT_POINTER(fobj->shared[0], fence); - fobj->shared_count = 1; - goto done; - } + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); + if (!new) + return -ENOMEM; /* * no need to bump fence refcounts, rcu_read access @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, * references from the old struct are carried over to * the new. */ - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { - struct dma_fence *check; + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { + struct dma_fence *fence; - check = rcu_dereference_protected(old->shared[i], - reservation_object_held(obj)); - - if (check->context == fence->context || - dma_fence_is_signaled(check)) - RCU_INIT_POINTER(fobj->shared[--k], check); + fence = rcu_dereference_protected(old->shared[i], + reservation_object_held(obj)); + if (dma_fence_is_signaled(fence)) + RCU_INIT_POINTER(new->shared[--k], fence); else - RCU_INIT_POINTER(fobj->shared[j++], check); + RCU_INIT_POINTER(new->shared[j++], fence); } - fobj->shared_count = j; - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); - fobj->shared_count++; + new->shared_count = j; + new->shared_max = max; -done: preempt_disable(); write_seqcount_begin(&obj->seq); /* * RCU_INIT_POINTER can be used here, * seqcount provides the necessary barriers */ - RCU_INIT_POINTER(obj->fence, fobj); + RCU_INIT_POINTER(obj->fence, new); write_seqcount_end(&obj->seq); preempt_enable(); if (!old) - return; + return 0; /* Drop the references to the signaled fences */ - for (i = k; i < fobj->shared_max; ++i) { - struct dma_fence *f; + for (i = k; i < new->shared_max; ++i) { + struct dma_fence *fence; - f = rcu_dereference_protected(fobj->shared[i], - reservation_object_held(obj)); - dma_fence_put(f); + fence = rcu_dereference_protected(new->shared[i], + reservation_object_held(obj)); + dma_fence_put(fence); } kfree_rcu(old, rcu); + + return 0; } +EXPORT_SYMBOL(reservation_object_reserve_shared); /** * reservation_object_add_shared_fence - Add a fence to a shared slot @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, void reservation_object_add_shared_fence(struct reservation_object *obj, struct dma_fence *fence) { - struct reservation_object_list *old, *fobj = obj->staged; + struct reservation_object_list *fobj; + unsigned int i; - old = reservation_object_get_list(obj); - obj->staged = NULL; + dma_fence_get(fence); + + fobj = reservation_object_get_list(obj); - if (!fobj) - reservation_object_add_shared_inplace(obj, old, fence); - else - reservation_object_add_shared_replace(obj, old, fobj, fence); + preempt_disable(); + write_seqcount_begin(&obj->seq); + + for (i = 0; i < fobj->shared_count; ++i) { + struct dma_fence *old_fence; + + old_fence = rcu_dereference_protected(fobj->shared[i], + reservation_object_held(obj)); + if (old_fence->context == fence->context || + dma_fence_is_signaled(old_fence)) { + dma_fence_put(old_fence); + goto replace; + } + } + + BUG_ON(fobj->shared_count >= fobj->shared_max); + fobj->shared_count++; + +replace: + /* + * memory barrier is added by write_seqcount_begin, + * fobj->shared_count is protected by this lock too + */ + RCU_INIT_POINTER(fobj->shared[i], fence); + write_seqcount_end(&obj->seq); + preempt_enable(); } EXPORT_SYMBOL(reservation_object_add_shared_fence); @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, new = dma_fence_get_rcu_safe(&src->fence_excl); rcu_read_unlock(); - kfree(dst->staged); - dst->staged = NULL; - src_list = reservation_object_get_list(dst); old = reservation_object_get_excl(dst); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -68,7 +68,6 @@ struct reservation_object_list { * @seq: sequence count for managing RCU read-side synchronization * @fence_excl: the exclusive fence, if there is one currently * @fence: list of current shared fences - * @staged: staged copy of shared fences for RCU updates */ struct reservation_object { struct ww_mutex lock; @@ -76,7 +75,6 @@ struct reservation_object { struct dma_fence __rcu *fence_excl; struct reservation_object_list __rcu *fence; - struct reservation_object_list *staged; }; #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); RCU_INIT_POINTER(obj->fence, NULL); RCU_INIT_POINTER(obj->fence_excl, NULL); - obj->staged = NULL; } /** @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) kfree(fobj); } - kfree(obj->staged); ww_mutex_destroy(&obj->lock); }
No need for that any more. Just replace the list when there isn't enough room any more for the additional fence. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- include/linux/reservation.h | 4 - 2 files changed, 58 insertions(+), 124 deletions(-)