The reservation object should be capable of handling its internal memory management itself. And since we search for a free slot to add the fence from the beginning this is actually a waste of time and only minimal helpful.
Drop it to allow removal of the seqno handling in the reservation object.
This essentially reverts commit "drm/i915: Remove completed fences after a wait".
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 27 ------------------------ 1 file changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 26ec6579b7cd..bb64ec6bef8e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -35,9 +35,7 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, unsigned int flags, long timeout) { - unsigned int seq = __read_seqcount_begin(&resv->seq); struct dma_fence *excl; - bool prune_fences = false;
if (flags & I915_WAIT_ALL) { struct dma_fence **shared; @@ -61,17 +59,6 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, for (; i < count; i++) dma_fence_put(shared[i]); kfree(shared); - - /* - * If both shared fences and an exclusive fence exist, - * then by construction the shared fences must be later - * than the exclusive fence. If we successfully wait for - * all the shared fences, we know that the exclusive fence - * must all be signaled. If all the shared fences are - * signaled, we can prune the array and recover the - * floating references on the fences/requests. - */ - prune_fences = count && timeout >= 0; } else { excl = reservation_object_get_excl_rcu(resv); } @@ -80,20 +67,6 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, timeout = i915_gem_object_wait_fence(excl, flags, timeout);
dma_fence_put(excl); - - /* - * Opportunistically prune the fences iff we know they have *all* been - * signaled and that the reservation object has not been changed (i.e. - * no new fences have been added). - */ - if (prune_fences && !__read_seqcount_retry(&resv->seq, seq)) { - if (reservation_object_trylock(resv)) { - if (!__read_seqcount_retry(&resv->seq, seq)) - reservation_object_add_excl_fence(resv, NULL); - reservation_object_unlock(resv); - } - } - return timeout; }
When reservation_object_add_shared_fence is replacing an old fence with a new one we should not drop the old one before the new one is in place.
Otherwise other cores can busy wait for the new one to appear.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/reservation.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index c71b85c8c159..d59207ca72d2 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -196,6 +196,7 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, struct dma_fence *fence) { struct reservation_object_list *fobj; + struct dma_fence *old; unsigned int i, count;
dma_fence_get(fence); @@ -209,18 +210,16 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, write_seqcount_begin(&obj->seq);
for (i = 0; i < 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); + old = rcu_dereference_protected(fobj->shared[i], + reservation_object_held(obj)); + if (old->context == fence->context || + dma_fence_is_signaled(old)) goto replace; - } }
BUG_ON(fobj->shared_count >= fobj->shared_max); + old = NULL; count++;
replace: @@ -230,6 +229,7 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
write_seqcount_end(&obj->seq); preempt_enable(); + dma_fence_put(old); } EXPORT_SYMBOL(reservation_object_add_shared_fence);
Other cores don't busy wait any more and we removed the last user of checking the seqno for changes. Drop updating the number for shared fences altogether.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/reservation.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index d59207ca72d2..6eaca469005f 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -206,9 +206,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, fobj = reservation_object_get_list(obj); count = fobj->shared_count;
- preempt_disable(); - write_seqcount_begin(&obj->seq); - for (i = 0; i < count; ++i) {
old = rcu_dereference_protected(fobj->shared[i], @@ -226,9 +223,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, RCU_INIT_POINTER(fobj->shared[i], fence); /* pointer update must be visible before we extend the shared_count */ smp_store_mb(fobj->shared_count, count); - - write_seqcount_end(&obj->seq); - preempt_enable(); dma_fence_put(old); } EXPORT_SYMBOL(reservation_object_add_shared_fence);
We can add the exclusive fence to the list after making sure we got a consistent state.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/reservation.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 6eaca469005f..69c826553c72 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -426,13 +426,6 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, if (!dma_fence_get_rcu(shared[i])) break; } - - if (!pfence_excl && fence_excl) { - shared[i] = fence_excl; - fence_excl = NULL; - ++i; - ++shared_count; - } }
if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { @@ -447,6 +440,11 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, rcu_read_unlock(); } while (ret);
+ if (pfence_excl) + *pfence_excl = fence_excl; + else if (fence_excl) + shared[++shared_count] = fence_excl; + if (!shared_count) { kfree(shared); shared = NULL; @@ -454,9 +452,6 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj,
*pshared_count = shared_count; *pshared = shared; - if (pfence_excl) - *pfence_excl = fence_excl; - return ret; } EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu);
The only remaining use for this is to protect against setting a new exclusive fence while we grab both exclusive and shared. That can also be archived by looking if the exclusive fence has changed or not after completing the operation.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 14 ++--- drivers/dma-buf/reservation.c | 58 +++++++------------ .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 +-- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 11 ++-- include/linux/reservation.h | 3 - 5 files changed, 33 insertions(+), 61 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f45bfb29ef96..c4ee4ccbfc40 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -199,7 +199,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) struct reservation_object_list *fobj; struct dma_fence *fence_excl; __poll_t events; - unsigned shared_count, seq; + unsigned shared_count;
dmabuf = file->private_data; if (!dmabuf || !dmabuf->resv) @@ -214,16 +214,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return 0;
retry: - seq = read_seqcount_begin(&resv->seq); rcu_read_lock();
+ fence_excl = rcu_dereference(resv->fence_excl); fobj = rcu_dereference(resv->fence); if (fobj) shared_count = fobj->shared_count; else shared_count = 0; - fence_excl = rcu_dereference(resv->fence_excl); - if (read_seqcount_retry(&resv->seq, seq)) { + + if (rcu_dereference(resv->fence_excl) != fence_excl) { rcu_read_unlock(); goto retry; } @@ -1157,7 +1157,6 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) struct reservation_object *robj; struct reservation_object_list *fobj; struct dma_fence *fence; - unsigned seq; int count = 0, attach_count, shared_count, i; size_t size = 0;
@@ -1189,12 +1188,11 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
robj = buf_obj->resv; while (true) { - seq = read_seqcount_begin(&robj->seq); rcu_read_lock(); + fence = rcu_dereference(robj->fence_excl); fobj = rcu_dereference(robj->fence); shared_count = fobj ? fobj->shared_count : 0; - fence = rcu_dereference(robj->fence_excl); - if (!read_seqcount_retry(&robj->seq, seq)) + if (rcu_dereference(robj->fence_excl) != fence) break; rcu_read_unlock(); } diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 69c826553c72..d8ed6235a3eb 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -49,12 +49,6 @@ DEFINE_WD_CLASS(reservation_ww_class); EXPORT_SYMBOL(reservation_ww_class);
-struct lock_class_key reservation_seqcount_class; -EXPORT_SYMBOL(reservation_seqcount_class); - -const char reservation_seqcount_string[] = "reservation_seqcount"; -EXPORT_SYMBOL(reservation_seqcount_string); - /** * reservation_object_init - initialize a reservation object * @obj: the reservation object @@ -62,9 +56,6 @@ EXPORT_SYMBOL(reservation_seqcount_string); void reservation_object_init(struct reservation_object *obj) { ww_mutex_init(&obj->lock, &reservation_ww_class); - - __seqcount_init(&obj->seq, reservation_seqcount_string, - &reservation_seqcount_class); RCU_INIT_POINTER(obj->fence, NULL); RCU_INIT_POINTER(obj->fence_excl, NULL); } @@ -251,12 +242,10 @@ void reservation_object_add_excl_fence(struct reservation_object *obj, dma_fence_get(fence);
preempt_disable(); - write_seqcount_begin(&obj->seq); - /* write_seqcount_begin provides the necessary memory barrier */ RCU_INIT_POINTER(obj->fence_excl, fence); + /* pointer update must be visible before we modify the shared_count */ if (old) - old->shared_count = 0; - write_seqcount_end(&obj->seq); + smp_store_mb(old->shared_count, 0); preempt_enable();
/* inplace update, no shared fences */ @@ -340,11 +329,8 @@ int reservation_object_copy_fences(struct reservation_object *dst, old = reservation_object_get_excl(dst);
preempt_disable(); - write_seqcount_begin(&dst->seq); - /* write_seqcount_begin provides the necessary memory barrier */ RCU_INIT_POINTER(dst->fence_excl, new); - RCU_INIT_POINTER(dst->fence, dst_list); - write_seqcount_end(&dst->seq); + rcu_assign_pointer(dst->fence, dst_list); preempt_enable();
if (src_list) @@ -380,18 +366,14 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj,
do { struct reservation_object_list *fobj; - unsigned int i, seq; + unsigned int i; size_t sz = 0;
shared_count = i = 0;
rcu_read_lock(); - seq = read_seqcount_begin(&obj->seq); - - fence_excl = rcu_dereference(obj->fence_excl); - if (fence_excl && !dma_fence_get_rcu(fence_excl)) - goto unlock;
+ fence_excl = dma_fence_get_rcu_safe(&obj->fence_excl); fobj = rcu_dereference(obj->fence); if (fobj) sz += sizeof(*shared) * fobj->shared_max; @@ -428,7 +410,8 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, } }
- if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { + if (i != shared_count || + rcu_dereference(obj->fence_excl) != fence_excl) { while (i--) dma_fence_put(shared[i]); dma_fence_put(fence_excl); @@ -472,18 +455,17 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, bool wait_all, bool intr, unsigned long timeout) { - struct dma_fence *fence; - unsigned seq, shared_count; + struct dma_fence *fence, *fence_excl; long ret = timeout ? timeout : 1; + unsigned shared_count; int i;
retry: shared_count = 0; - seq = read_seqcount_begin(&obj->seq); rcu_read_lock(); i = -1;
- fence = rcu_dereference(obj->fence_excl); + fence = fence_excl = rcu_dereference(obj->fence_excl); if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { if (!dma_fence_get_rcu(fence)) goto unlock_retry; @@ -524,13 +506,13 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, } }
+ if (rcu_dereference(obj->fence_excl) != fence_excl) { + dma_fence_put(fence); + goto unlock_retry; + } + rcu_read_unlock(); if (fence) { - if (read_seqcount_retry(&obj->seq, seq)) { - dma_fence_put(fence); - goto retry; - } - ret = dma_fence_wait_timeout(fence, intr, ret); dma_fence_put(fence); if (ret > 0 && wait_all && (i + 1 < shared_count)) @@ -575,14 +557,15 @@ reservation_object_test_signaled_single(struct dma_fence *passed_fence) bool reservation_object_test_signaled_rcu(struct reservation_object *obj, bool test_all) { - unsigned seq, shared_count; + struct dma_fence *fence_excl; + unsigned shared_count; int ret;
rcu_read_lock(); retry: + fence_excl = rcu_dereference(obj->fence_excl); ret = true; shared_count = 0; - seq = read_seqcount_begin(&obj->seq);
if (test_all) { unsigned i; @@ -603,12 +586,11 @@ bool reservation_object_test_signaled_rcu(struct reservation_object *obj, break; }
- if (read_seqcount_retry(&obj->seq, seq)) + if (rcu_dereference(obj->fence_excl) != fence_excl) goto retry; }
if (!shared_count) { - struct dma_fence *fence_excl = rcu_dereference(obj->fence_excl);
if (fence_excl) { ret = reservation_object_test_signaled_single( @@ -616,7 +598,7 @@ bool reservation_object_test_signaled_rcu(struct reservation_object *obj, if (ret < 0) goto retry;
- if (read_seqcount_retry(&obj->seq, seq)) + if (rcu_dereference(obj->fence_excl) != fence_excl) goto retry; } } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 1d3ee9c42f7e..a2a2ae592f20 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -251,12 +251,8 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, new->shared_max = old->shared_max; new->shared_count = k;
- /* Install the new fence list, seqcount provides the barriers */ - preempt_disable(); - write_seqcount_begin(&resv->seq); - RCU_INIT_POINTER(resv->fence, new); - write_seqcount_end(&resv->seq); - preempt_enable(); + /* Install the new fence list */ + rcu_assign_pointer(resv->fence, new);
/* Drop the references to the removed fences or move them to ef_list */ for (i = j, k = 0; i < old->shared_count; ++i) { diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6ad93a09968c..6e8a6e4f39ff 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -83,7 +83,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; struct reservation_object_list *list; - unsigned int seq; + struct dma_fence *fence_excl; int err;
err = -ENOENT; @@ -110,11 +110,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * */ retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = - busy_check_writer(rcu_dereference(obj->base.resv->fence_excl)); + fence_excl = rcu_dereference(obj->base.resv->fence_excl); + args->busy = busy_check_writer(fence_excl);
/* Translate shared fences to READ set of engines */ list = rcu_dereference(obj->base.resv->fence); @@ -129,7 +127,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, } }
- if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) + if (args->busy && + rcu_dereference(obj->base.resv->fence_excl) != fence_excl) goto retry;
err = 0; diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 56b782fec49b..2b0b2a1aeae2 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -46,8 +46,6 @@ #include <linux/rcupdate.h>
extern struct ww_class reservation_ww_class; -extern struct lock_class_key reservation_seqcount_class; -extern const char reservation_seqcount_string[];
/** * struct reservation_object_list - a list of shared fences @@ -71,7 +69,6 @@ struct reservation_object_list { */ struct reservation_object { struct ww_mutex lock; - seqcount_t seq;
struct dma_fence __rcu *fence_excl; struct reservation_object_list __rcu *fence;
Quoting Christian König (2019-08-05 16:45:54)
@@ -214,16 +214,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return 0;
retry:
seq = read_seqcount_begin(&resv->seq); rcu_read_lock();
fence_excl = rcu_dereference(resv->fence_excl); fobj = rcu_dereference(resv->fence); if (fobj) shared_count = fobj->shared_count; else shared_count = 0;
fence_excl = rcu_dereference(resv->fence_excl);if (read_seqcount_retry(&resv->seq, seq)) {
if (rcu_dereference(resv->fence_excl) != fence_excl) {
If I remember my rules correctly, rcu_dereference is a read-data-depends, which only means that a read through the pointer returned by rcu_dereference() is after the retrieval of that pointer. Nothing orders the retrieval of fence_excl vs shared_count (different pointers), so I think the last line should be:
smp_rmb(); if (rcu_access_pointer(resv->fence_excl) != fence_excl) -Chris
On Mon, Aug 05, 2019 at 05:07:41PM +0100, Chris Wilson wrote:
Quoting Christian König (2019-08-05 16:45:54)
@@ -214,16 +214,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return 0;
retry:
seq = read_seqcount_begin(&resv->seq); rcu_read_lock();
fence_excl = rcu_dereference(resv->fence_excl); fobj = rcu_dereference(resv->fence); if (fobj) shared_count = fobj->shared_count; else shared_count = 0;
fence_excl = rcu_dereference(resv->fence_excl);if (read_seqcount_retry(&resv->seq, seq)) {
if (rcu_dereference(resv->fence_excl) != fence_excl) {If I remember my rules correctly, rcu_dereference is a read-data-depends, which only means that a read through the pointer returned by rcu_dereference() is after the retrieval of that pointer. Nothing orders the retrieval of fence_excl vs shared_count (different pointers), so I think the last line should be:
smp_rmb(); if (rcu_access_pointer(resv->fence_excl) != fence_excl)
Also, barriers must have a comment, the comment must be on both barriers (if you don't have two, you're doing it wrong), and each barrier comment must reference its counterpart. -Daniel
Quoting Christian König (2019-08-05 16:45:50)
The reservation object should be capable of handling its internal memory management itself. And since we search for a free slot to add the fence from the beginning this is actually a waste of time and only minimal helpful.
"From the beginning?" Attempting to prune signaled fences on insertion is quite recent.
However, that doesn't help the cases where reservation_object keeps on holding a reference to the fences for idle objects. It's an absolute nightmare of a reference trap. -Chris
Quoting Chris Wilson (2019-08-05 16:58:56)
Quoting Christian König (2019-08-05 16:45:50)
The reservation object should be capable of handling its internal memory management itself. And since we search for a free slot to add the fence from the beginning this is actually a waste of time and only minimal helpful.
"From the beginning?" Attempting to prune signaled fences on insertion is quite recent.
However, that doesn't help the cases where reservation_object keeps on holding a reference to the fences for idle objects. It's an absolute nightmare of a reference trap.
Fwiw, it's a pet peeve, and not a fundamental object to removing some loops inside reservation_object. Here, the seqno is being used as a guide to avoid trying to take the lock if it's been externally modified, but it would equally work with just a plain trylock + test_rcu.
Better yet would be autopruning, but that suggests a slightly different data structure an rbtree instead of an array and spinlocked cb_list manipulation instead of a plain refcount. -Chris
Am 05.08.19 um 18:54 schrieb Chris Wilson:
Quoting Chris Wilson (2019-08-05 16:58:56)
Quoting Christian König (2019-08-05 16:45:50)
The reservation object should be capable of handling its internal memory management itself. And since we search for a free slot to add the fence from the beginning this is actually a waste of time and only minimal helpful.
"From the beginning?" Attempting to prune signaled fences on insertion is quite recent.
What I meant was from the beginning of the array :) Sorry for the confusion.
However, that doesn't help the cases where reservation_object keeps on holding a reference to the fences for idle objects. It's an absolute nightmare of a reference trap.
We only free up the fence objects, but not the array itself. And userspace actually needs to call the wait function.
So to me this looks like it doesn't really helps much.
Fwiw, it's a pet peeve, and not a fundamental object to removing some loops inside reservation_object. Here, the seqno is being used as a guide to avoid trying to take the lock if it's been externally modified, but it would equally work with just a plain trylock + test_rcu.
Ok, going to do this then for the meantime.
Better yet would be autopruning, but that suggests a slightly different data structure an rbtree instead of an array and spinlocked cb_list manipulation instead of a plain refcount.
Yeah, that's exactly what I'm working on with this series.
Regards, Christian.
-Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Christian König -
Daniel Vetter