Add a new helper to get a consistent set of pointers from the reservation object. While at it group all access helpers together in the header file.
v2: correctly return shared_count as well
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 31 ++------- drivers/dma-buf/reservation.c | 82 ++++++++---------------- include/linux/reservation.h | 115 +++++++++++++++++++++------------- 3 files changed, 101 insertions(+), 127 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f45bfb29ef96..67510f2be8bc 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) @@ -213,21 +213,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) if (!events) return 0;
-retry: - seq = read_seqcount_begin(&resv->seq); rcu_read_lock(); - - 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)) { - rcu_read_unlock(); - goto retry; - } - + reservation_object_fences(resv, &fence_excl, &fobj, &shared_count); if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) { struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl; __poll_t pevents = EPOLLIN; @@ -1157,7 +1144,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;
@@ -1188,16 +1174,9 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) buf_obj->name ?: "");
robj = buf_obj->resv; - while (true) { - seq = read_seqcount_begin(&robj->seq); - rcu_read_lock(); - 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)) - break; - rcu_read_unlock(); - } + rcu_read_lock(); + reservation_object_fences(robj, &fence, &fobj, &shared_count); + rcu_read_unlock();
if (fence) seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index ad6775b32a73..8fcaddffd5d4 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -317,17 +317,15 @@ int reservation_object_copy_fences(struct reservation_object *dst, { struct reservation_object_list *src_list, *dst_list; struct dma_fence *old, *new; - unsigned i; + unsigned int i, shared_count;
reservation_object_assert_held(dst);
rcu_read_lock(); - src_list = rcu_dereference(src->fence);
retry: - if (src_list) { - unsigned shared_count = src_list->shared_count; - + reservation_object_fences(src, &new, &src_list, &shared_count); + if (shared_count) { rcu_read_unlock();
dst_list = reservation_object_list_alloc(shared_count); @@ -335,14 +333,14 @@ int reservation_object_copy_fences(struct reservation_object *dst, return -ENOMEM;
rcu_read_lock(); - src_list = rcu_dereference(src->fence); - if (!src_list || src_list->shared_count > shared_count) { + reservation_object_fences(src, &new, &src_list, &shared_count); + if (!src_list || shared_count > dst_list->shared_max) { kfree(dst_list); goto retry; }
dst_list->shared_count = 0; - for (i = 0; i < src_list->shared_count; ++i) { + for (i = 0; i < shared_count; ++i) { struct dma_fence *fence;
fence = rcu_dereference(src_list->shared[i]); @@ -352,7 +350,6 @@ int reservation_object_copy_fences(struct reservation_object *dst,
if (!dma_fence_get_rcu(fence)) { reservation_object_list_free(dst_list); - src_list = rcu_dereference(src->fence); goto retry; }
@@ -367,7 +364,10 @@ int reservation_object_copy_fences(struct reservation_object *dst, dst_list = NULL; }
- new = dma_fence_get_rcu_safe(&src->fence_excl); + if (new && !dma_fence_get_rcu(new)) { + reservation_object_list_free(dst_list); + goto retry; + } rcu_read_unlock();
src_list = reservation_object_get_list(dst); @@ -413,19 +413,18 @@ 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; + i = 0;
rcu_read_lock(); - seq = read_seqcount_begin(&obj->seq); + reservation_object_fences(obj, &fence_excl, &fobj, + &shared_count);
- fence_excl = rcu_dereference(obj->fence_excl); if (fence_excl && !dma_fence_get_rcu(fence_excl)) goto unlock;
- fobj = rcu_dereference(obj->fence); if (fobj) sz += sizeof(*shared) * fobj->shared_max;
@@ -453,7 +452,6 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, break; } shared = nshared; - shared_count = fobj ? fobj->shared_count : 0; for (i = 0; i < shared_count; ++i) { shared[i] = rcu_dereference(fobj->shared[i]); if (!dma_fence_get_rcu(shared[i])) @@ -461,7 +459,7 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, } }
- if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { + if (i != shared_count) { while (i--) dma_fence_put(shared[i]); dma_fence_put(fence_excl); @@ -505,18 +503,17 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, bool wait_all, bool intr, unsigned long timeout) { + struct reservation_object_list *fobj; struct dma_fence *fence; - unsigned seq, shared_count; + unsigned shared_count; long ret = timeout ? timeout : 1; int i;
retry: - shared_count = 0; - seq = read_seqcount_begin(&obj->seq); rcu_read_lock(); i = -1;
- fence = rcu_dereference(obj->fence_excl); + reservation_object_fences(obj, &fence, &fobj, &shared_count); if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { if (!dma_fence_get_rcu(fence)) goto unlock_retry; @@ -531,12 +528,6 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, }
if (wait_all) { - struct reservation_object_list *fobj = - rcu_dereference(obj->fence); - - if (fobj) - shared_count = fobj->shared_count; - for (i = 0; !fence && i < shared_count; ++i) { struct dma_fence *lfence = rcu_dereference(fobj->shared[i]);
@@ -559,11 +550,6 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
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)) @@ -608,24 +594,19 @@ 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 reservation_object_list *fobj; + struct dma_fence *fence_excl; + unsigned shared_count; int ret;
rcu_read_lock(); retry: ret = true; - shared_count = 0; - seq = read_seqcount_begin(&obj->seq);
+ reservation_object_fences(obj, &fence_excl, &fobj, &shared_count); if (test_all) { unsigned i;
- struct reservation_object_list *fobj = - rcu_dereference(obj->fence); - - if (fobj) - shared_count = fobj->shared_count; - for (i = 0; i < shared_count; ++i) { struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
@@ -635,23 +616,12 @@ bool reservation_object_test_signaled_rcu(struct reservation_object *obj, else if (!ret) break; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; }
- if (!shared_count) { - struct dma_fence *fence_excl = rcu_dereference(obj->fence_excl); - - if (fence_excl) { - ret = reservation_object_test_signaled_single( - fence_excl); - if (ret < 0) - goto retry; - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - } + if (!shared_count && fence_excl) { + ret = reservation_object_test_signaled_single(fence_excl); + if (ret < 0) + goto retry; }
rcu_read_unlock(); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 56b782fec49b..044a5cd4af50 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -81,6 +81,51 @@ struct reservation_object { #define reservation_object_assert_held(obj) \ lockdep_assert_held(&(obj)->lock.base)
+/** + * reservation_object_get_excl - get the reservation object's + * exclusive fence, with update-side lock held + * @obj: the reservation object + * + * Returns the exclusive fence (if any). Does NOT take a + * reference. Writers must hold obj->lock, readers may only + * hold a RCU read side lock. + * + * RETURNS + * The exclusive fence or NULL + */ +static inline struct dma_fence * +reservation_object_get_excl(struct reservation_object *obj) +{ + return rcu_dereference_protected(obj->fence_excl, + reservation_object_held(obj)); +} + +/** + * reservation_object_get_excl_rcu - get the reservation object's + * exclusive fence, without lock held. + * @obj: the reservation object + * + * If there is an exclusive fence, this atomically increments it's + * reference count and returns it. + * + * RETURNS + * The exclusive fence or NULL if none + */ +static inline struct dma_fence * +reservation_object_get_excl_rcu(struct reservation_object *obj) +{ + struct dma_fence *fence; + + if (!rcu_access_pointer(obj->fence_excl)) + return NULL; + + rcu_read_lock(); + fence = dma_fence_get_rcu_safe(&obj->fence_excl); + rcu_read_unlock(); + + return fence; +} + /** * reservation_object_get_list - get the reservation object's * shared fence list, with update-side lock held @@ -96,6 +141,31 @@ reservation_object_get_list(struct reservation_object *obj) reservation_object_held(obj)); }
+/** + * reservation_object_fences - read consistent fence pointers + * @obj: reservation object where we get the fences from + * @excl: pointer for the exclusive fence + * @list: pointer for the shared fence list + * + * Make sure we have a consisten exclusive fence and shared fence list. + * Must be called with rcu read side lock held. + */ +static inline void +reservation_object_fences(struct reservation_object *obj, + struct dma_fence **excl, + struct reservation_object_list **list, + u32 *shared_count) +{ + unsigned int seq; + + do { + seq = read_seqcount_begin(&obj->seq); + *excl = rcu_dereference(obj->fence_excl); + *list = rcu_dereference(obj->fence); + *shared_count = *list ? (*list)->shared_count : 0; + } while (read_seqcount_retry(&obj->seq, seq)); +} + /** * reservation_object_lock - lock the reservation object * @obj: the reservation object @@ -239,51 +309,6 @@ reservation_object_unlock(struct reservation_object *obj) ww_mutex_unlock(&obj->lock); }
-/** - * reservation_object_get_excl - get the reservation object's - * exclusive fence, with update-side lock held - * @obj: the reservation object - * - * Returns the exclusive fence (if any). Does NOT take a - * reference. Writers must hold obj->lock, readers may only - * hold a RCU read side lock. - * - * RETURNS - * The exclusive fence or NULL - */ -static inline struct dma_fence * -reservation_object_get_excl(struct reservation_object *obj) -{ - return rcu_dereference_protected(obj->fence_excl, - reservation_object_held(obj)); -} - -/** - * reservation_object_get_excl_rcu - get the reservation object's - * exclusive fence, without lock held. - * @obj: the reservation object - * - * If there is an exclusive fence, this atomically increments it's - * reference count and returns it. - * - * RETURNS - * The exclusive fence or NULL if none - */ -static inline struct dma_fence * -reservation_object_get_excl_rcu(struct reservation_object *obj) -{ - struct dma_fence *fence; - - if (!rcu_access_pointer(obj->fence_excl)) - return NULL; - - rcu_read_lock(); - fence = dma_fence_get_rcu_safe(&obj->fence_excl); - rcu_read_unlock(); - - return fence; -} - void reservation_object_init(struct reservation_object *obj); void reservation_object_fini(struct reservation_object *obj); int reservation_object_reserve_shared(struct reservation_object *obj,
Instead of open coding the sequence loop use the new helper.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6ad93a09968c..8473292096cb 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -83,7 +83,8 @@ 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; + unsigned int i, shared_count; + struct dma_fence *excl; int err;
err = -ENOENT; @@ -109,29 +110,18 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); + reservation_object_fences(obj->base.resv, &excl, &list, &shared_count);
/* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = - busy_check_writer(rcu_dereference(obj->base.resv->fence_excl)); + args->busy = busy_check_writer(excl);
/* Translate shared fences to READ set of engines */ - list = rcu_dereference(obj->base.resv->fence); - if (list) { - unsigned int shared_count = list->shared_count, i; + for (i = 0; i < shared_count; ++i) { + struct dma_fence *fence = rcu_dereference(list->shared[i]);
- for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - - args->busy |= busy_check_reader(fence); - } + args->busy |= busy_check_reader(fence); }
- if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; - err = 0; out: rcu_read_unlock();
Quoting Christian König (2019-08-07 14:53:10)
Instead of open coding the sequence loop use the new helper.
Signed-off-by: Christian König christian.koenig@amd.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
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 ------ drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 7 +------ 2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 8fcaddffd5d4..90bc6ef03598 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -237,9 +237,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], @@ -257,9 +254,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); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index fe062b76ec91..a4640ddc24d1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -251,12 +251,7 @@ 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(); + 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) {
Quoting Christian König (2019-08-07 14:53:11)
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
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
drivers/dma-buf/reservation.c | 6 ------ drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 7 +------ 2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 8fcaddffd5d4..90bc6ef03598 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -237,9 +237,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], @@ -257,9 +254,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); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index fe062b76ec91..a4640ddc24d1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -251,12 +251,7 @@ 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();
rcu_assign_pointer(resv->fence, new);
but you'll probably want a local ack for amdgpu/ -Chris
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list * @shared_max: number of fences we need space for @@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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); } @@ -282,12 +273,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); + rcu_assign_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 */ @@ -368,11 +357,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_excl, new); + rcu_assign_pointer(dst->fence, dst_list); preempt_enable();
reservation_object_list_free(src_list); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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; @@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) { - unsigned int seq; - do { - seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl); *list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; - } while (read_seqcount_retry(&obj->seq, seq)); + smp_rmb(); /* See reservation_object_add_excl_fence */ + } while (rcu_access_pointer(obj->fence_excl) != *excl); }
/**
Quoting Christian König (2019-08-07 14:53:12)
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list
- @shared_max: number of fences we need space for
@@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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);} @@ -282,12 +273,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);
rcu_assign_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 */ @@ -368,11 +357,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_excl, new);rcu_assign_pointer(dst->fence, dst_list); preempt_enable();reservation_object_list_free(src_list); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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; @@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) {
unsigned int seq;do {seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl); *list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0;} while (read_seqcount_retry(&obj->seq, seq));
smp_rmb(); /* See reservation_object_add_excl_fence */} while (rcu_access_pointer(obj->fence_excl) != *excl);}
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
I think this is correct. Now see if we can convince Daniel! -Chris
Am 07.08.19 um 16:17 schrieb Chris Wilson:
Quoting Christian König (2019-08-07 14:53:12)
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list
- @shared_max: number of fences we need space for
@@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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);@@ -282,12 +273,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);
rcu_assign_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 */ @@ -368,11 +357,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_excl, new);rcu_assign_pointer(dst->fence, dst_list); preempt_enable();reservation_object_list_free(src_list); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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; @@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) {
unsigned int seq;do {seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl); *list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0;} while (read_seqcount_retry(&obj->seq, seq));
smp_rmb(); /* See reservation_object_add_excl_fence */} while (rcu_access_pointer(obj->fence_excl) != *excl);Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
I think this is correct. Now see if we can convince Daniel!
Daniel any objections to this? IGTs look good as well, so if not I'm going to push it.
Christian.
-Chris
Sorry I burried myself in some other stuff ...
On Sat, Aug 10, 2019 at 12:51:00PM +0200, Christian König wrote:
Am 07.08.19 um 16:17 schrieb Chris Wilson:
Quoting Christian König (2019-08-07 14:53:12)
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list
- @shared_max: number of fences we need space for
@@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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);@@ -282,12 +273,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);
rcu_assign_pointer(obj->fence_excl, fence);/* pointer update must be visible before we modify the shared_count */
Pls add a "see reservation_object_fence()" here or similar.
if (old)
old->shared_count = 0;write_seqcount_end(&obj->seq);
smp_store_mb(old->shared_count, 0);
So your comment and the kerneldoc don't match up. Quoting Documentation/memory-barriers.txt:
This assigns the value to the variable and then inserts a full memory barrier after it. It isn't guaranteed to insert anything more than a compiler barrier in a UP compilation.
So order is 1. store 2. fence, but your comment suggests you want it the other way round.
preempt_enable(); /* inplace update, no shared fences */@@ -368,11 +357,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_excl, new);rcu_assign_pointer(dst->fence, dst_list); preempt_enable(); reservation_object_list_free(src_list);diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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;@@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) {
unsigned int seq;do {seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl);
I think you need a barrier between this and the read of shared_count below. But rcu_derefence only gives you a dependent barrier, i.e. only stuff that's accesses through this pointer is ordered. Which means the access to ->shared_count, which goes through another pointer, isn't actually ordered.
I think the implementation is that it is an unconditional compiler barrier (but that might change), but you're definitely missing the cpu barrier, so a cpue might speculate the entire thing out of order.
I think you need another smb_rmb(); here
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0;
} while (read_seqcount_retry(&obj->seq, seq));
smp_rmb(); /* See reservation_object_add_excl_fence */
This fence here I think prevents the re-reading of ->fence_excl from getting hoisted above the critical reads. So this is just the open-coded seqlock retry loop.
} while (rcu_access_pointer(obj->fence_excl) != *excl);
What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
}
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
I think this is correct. Now see if we can convince Daniel!
Daniel any objections to this? IGTs look good as well, so if not I'm going to push it.
Not really convinced. Also haven't looked at the entire thing yet, this is just from staring at this patch in isolation and poking at it. -Daniel
Quoting Daniel Vetter (2019-08-14 16:39:08)
Sorry I burried myself in some other stuff ...
On Sat, Aug 10, 2019 at 12:51:00PM +0200, Christian König wrote:
Am 07.08.19 um 16:17 schrieb Chris Wilson:
Quoting Christian König (2019-08-07 14:53:12)
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list
- @shared_max: number of fences we need space for
@@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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);@@ -282,12 +273,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);
rcu_assign_pointer(obj->fence_excl, fence);/* pointer update must be visible before we modify the shared_count */Pls add a "see reservation_object_fence()" here or similar.
if (old)
old->shared_count = 0;write_seqcount_end(&obj->seq);
smp_store_mb(old->shared_count, 0);So your comment and the kerneldoc don't match up. Quoting Documentation/memory-barriers.txt:
This assigns the value to the variable and then inserts a full memory barrier after it. It isn't guaranteed to insert anything more than a compiler barrier in a UP compilation.So order is 1. store 2. fence, but your comment suggests you want it the other way round.
What's more weird is that it is a fully serialising instruction that is used to fence first as part of the update. If that's way PeterZ uses it...
preempt_enable(); /* inplace update, no shared fences */@@ -368,11 +357,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_excl, new);rcu_assign_pointer(dst->fence, dst_list); preempt_enable(); reservation_object_list_free(src_list);diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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;@@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) {
unsigned int seq;do {seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl);I think you need a barrier between this and the read of shared_count below. But rcu_derefence only gives you a dependent barrier, i.e. only stuff that's accesses through this pointer is ordered. Which means the access to ->shared_count, which goes through another pointer, isn't actually ordered.
Well,
do { excl = ... smp_rmb(); (list, count) = ... smp_rmb(); } while (excl != ...)
would be the straightforward conversion of the seqlock.
I think the implementation is that it is an unconditional compiler barrier (but that might change), but you're definitely missing the cpu barrier, so a cpue might speculate the entire thing out of order.
I think you need another smb_rmb(); here
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0;
} while (read_seqcount_retry(&obj->seq, seq));
smp_rmb(); /* See reservation_object_add_excl_fence */This fence here I think prevents the re-reading of ->fence_excl from getting hoisted above the critical reads. So this is just the open-coded seqlock retry loop.
Without the seq.
The dilemma for dropping the seq would be what if we were to add another state here, such as modified or even invalidate.
} while (rcu_access_pointer(obj->fence_excl) != *excl);What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot. -Chris
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08)
} while (rcu_access_pointer(obj->fence_excl) != *excl);What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl. -Chris
Quoting Chris Wilson (2019-08-14 18:06:18)
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08)
} while (rcu_access_pointer(obj->fence_excl) != *excl);What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl.
The problem is that the point of the loop is that we do need a check on the fences after the full memory barrier.
Thinking of the rationale beaten out for dma_fence_get_excl_rcu_safe()
We don't have a full memory barrier here, so this cannot be used safely in light of fence reallocation. -Chris
Quoting Chris Wilson (2019-08-14 18:22:53)
Quoting Chris Wilson (2019-08-14 18:06:18)
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08)
> + } while (rcu_access_pointer(obj->fence_excl) != *excl);
What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl.
The problem is that the point of the loop is that we do need a check on the fences after the full memory barrier.
Thinking of the rationale beaten out for dma_fence_get_excl_rcu_safe()
We don't have a full memory barrier here, so this cannot be used safely in light of fence reallocation.
i.e. we need to restore the loops in the callers,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index a2aff1d8290e..f019062c8cd7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -110,6 +110,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ +retry: dma_resv_fences(obj->base.resv, &excl, &list, &shared_count);
/* Translate the exclusive fence to the READ *and* WRITE engine */ @@ -122,6 +123,10 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, args->busy |= busy_check_reader(fence); }
+ smp_rmb(); + if (excl != rcu_access_pointer(obj->base.resv->fence_excl)) + goto retry; +
wrap that up as
static inline bool dma_resv_fences_retry(struct dma_resv *resv, struct dma_fence *excl) { smp_rmb(); return excl != rcu_access_pointer(resv->fence_excl); } -Chris
Quoting Chris Wilson (2019-08-14 18:38:20)
Quoting Chris Wilson (2019-08-14 18:22:53)
Quoting Chris Wilson (2019-08-14 18:06:18)
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08)
> > + } while (rcu_access_pointer(obj->fence_excl) != *excl);
What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl.
The problem is that the point of the loop is that we do need a check on the fences after the full memory barrier.
Thinking of the rationale beaten out for dma_fence_get_excl_rcu_safe()
We don't have a full memory barrier here, so this cannot be used safely in light of fence reallocation.
i.e. we need to restore the loops in the callers,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index a2aff1d8290e..f019062c8cd7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -110,6 +110,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ +retry: dma_resv_fences(obj->base.resv, &excl, &list, &shared_count);
/* Translate the exclusive fence to the READ *and* WRITE engine */@@ -122,6 +123,10 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, args->busy |= busy_check_reader(fence); }
smp_rmb();if (excl != rcu_access_pointer(obj->base.resv->fence_excl))goto retry;wrap that up as
static inline bool dma_resv_fences_retry(struct dma_resv *resv, struct dma_fence *excl) { smp_rmb(); return excl != rcu_access_pointer(resv->fence_excl); }
I give up. It's not just the fence_excl that's an issue here.
Any of the shared fences may be replaced after dma_resv_fences() and so the original shared fence pointer may be reassigned (even under RCU). The only defense against that is the seqcount.
I totally screwed that up. -Chris
Am 14.08.19 um 19:48 schrieb Chris Wilson:
Quoting Chris Wilson (2019-08-14 18:38:20)
Quoting Chris Wilson (2019-08-14 18:22:53)
Quoting Chris Wilson (2019-08-14 18:06:18)
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08)
>>> + } while (rcu_access_pointer(obj->fence_excl) != *excl); What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl.
The problem is that the point of the loop is that we do need a check on the fences after the full memory barrier.
Thinking of the rationale beaten out for dma_fence_get_excl_rcu_safe()
We don't have a full memory barrier here, so this cannot be used safely in light of fence reallocation.
i.e. we need to restore the loops in the callers,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index a2aff1d8290e..f019062c8cd7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -110,6 +110,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ +retry: dma_resv_fences(obj->base.resv, &excl, &list, &shared_count);
/* Translate the exclusive fence to the READ *and* WRITE engine */@@ -122,6 +123,10 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, args->busy |= busy_check_reader(fence); }
smp_rmb();if (excl != rcu_access_pointer(obj->base.resv->fence_excl))goto retry;wrap that up as
static inline bool dma_resv_fences_retry(struct dma_resv *resv, struct dma_fence *excl) { smp_rmb(); return excl != rcu_access_pointer(resv->fence_excl); }
I give up. It's not just the fence_excl that's an issue here.
Any of the shared fences may be replaced after dma_resv_fences() and so the original shared fence pointer may be reassigned (even under RCU).
Yeah, but this should be harmless. See fences are always replaced either when they are signaled anyway or by later fences from the same context.
And existing fences shouldn't be re-used while under RCU, or is anybody still using SLAB_TYPESAFE_BY_RCU?
Christian.
The only defense against that is the seqcount.
I totally screwed that up. -Chris
Quoting Koenig, Christian (2019-08-14 18:58:32)
Am 14.08.19 um 19:48 schrieb Chris Wilson:
Quoting Chris Wilson (2019-08-14 18:38:20)
Quoting Chris Wilson (2019-08-14 18:22:53)
Quoting Chris Wilson (2019-08-14 18:06:18)
Quoting Chris Wilson (2019-08-14 17:42:48)
Quoting Daniel Vetter (2019-08-14 16:39:08) >>>> + } while (rcu_access_pointer(obj->fence_excl) != *excl); > What if someone is real fast (like really real fast) and recycles the > exclusive fence so you read the same pointer twice, but everything else > changed? reused fence pointer is a lot more likely than seqlock wrapping > around. It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
An extension of that argument says we don't even need to loop,
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0; smp_rmb(); *excl = rcu_dereference(obj->fence_excl);
Gives a consistent snapshot. It doesn't matter if the fence_excl is before or after the shared_list -- if it's after, it's a superset of all fences, if it's before, we have a correct list of shared fences the earlier fence_excl.
The problem is that the point of the loop is that we do need a check on the fences after the full memory barrier.
Thinking of the rationale beaten out for dma_fence_get_excl_rcu_safe()
We don't have a full memory barrier here, so this cannot be used safely in light of fence reallocation.
i.e. we need to restore the loops in the callers,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index a2aff1d8290e..f019062c8cd7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -110,6 +110,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ +retry: dma_resv_fences(obj->base.resv, &excl, &list, &shared_count);
/* Translate the exclusive fence to the READ *and* WRITE engine */@@ -122,6 +123,10 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, args->busy |= busy_check_reader(fence); }
smp_rmb();if (excl != rcu_access_pointer(obj->base.resv->fence_excl))goto retry;wrap that up as
static inline bool dma_resv_fences_retry(struct dma_resv *resv, struct dma_fence *excl) { smp_rmb(); return excl != rcu_access_pointer(resv->fence_excl); }
I give up. It's not just the fence_excl that's an issue here.
Any of the shared fences may be replaced after dma_resv_fences() and so the original shared fence pointer may be reassigned (even under RCU).
Yeah, but this should be harmless. See fences are always replaced either when they are signaled anyway or by later fences from the same context.
And existing fences shouldn't be re-used while under RCU, or is anybody still using SLAB_TYPESAFE_BY_RCU?
Yes. We go through enough fences that the freelist is a noticeable improvement. -Chris
On Wed, Aug 14, 2019 at 05:42:48PM +0100, Chris Wilson wrote:
Quoting Daniel Vetter (2019-08-14 16:39:08)
Sorry I burried myself in some other stuff ...
On Sat, Aug 10, 2019 at 12:51:00PM +0200, Christian König wrote:
Am 07.08.19 um 16:17 schrieb Chris Wilson:
Quoting Christian König (2019-08-07 14:53:12)
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.
v2: switch setting excl fence to rcu_assign_pointer
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/reservation.c | 24 +++++------------------- include/linux/reservation.h | 9 ++------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 90bc6ef03598..f7f4a0858c2a 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_list_alloc - allocate fence list
- @shared_max: number of fences we need space for
@@ -103,9 +97,6 @@ static void reservation_object_list_free(struct reservation_object_list *list) 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);@@ -282,12 +273,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);
rcu_assign_pointer(obj->fence_excl, fence);/* pointer update must be visible before we modify the shared_count */Pls add a "see reservation_object_fence()" here or similar.
if (old)
old->shared_count = 0;write_seqcount_end(&obj->seq);
smp_store_mb(old->shared_count, 0);So your comment and the kerneldoc don't match up. Quoting Documentation/memory-barriers.txt:
This assigns the value to the variable and then inserts a full memory barrier after it. It isn't guaranteed to insert anything more than a compiler barrier in a UP compilation.So order is 1. store 2. fence, but your comment suggests you want it the other way round.
What's more weird is that it is a fully serialising instruction that is used to fence first as part of the update. If that's way PeterZ uses it...
I haven't looked at the implementations tbh, just going with the text. Or do you mean in the write_seqlock that we're replacing?
preempt_enable(); /* inplace update, no shared fences */@@ -368,11 +357,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_excl, new);rcu_assign_pointer(dst->fence, dst_list); preempt_enable(); reservation_object_list_free(src_list);diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 044a5cd4af50..fd29baad0be3 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;@@ -156,14 +153,12 @@ reservation_object_fences(struct reservation_object *obj, struct reservation_object_list **list, u32 *shared_count) {
unsigned int seq;do {seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl);I think you need a barrier between this and the read of shared_count below. But rcu_derefence only gives you a dependent barrier, i.e. only stuff that's accesses through this pointer is ordered. Which means the access to ->shared_count, which goes through another pointer, isn't actually ordered.
Well,
do { excl = ... smp_rmb(); (list, count) = ... smp_rmb(); } while (excl != ...)
would be the straightforward conversion of the seqlock.
Yeah I cheated by looking there, and couldn't convince myself that we can't drop the first smp_rmb() ...
I think the implementation is that it is an unconditional compiler barrier (but that might change), but you're definitely missing the cpu barrier, so a cpue might speculate the entire thing out of order.
I think you need another smb_rmb(); here
*list = rcu_dereference(obj->fence); *shared_count = *list ? (*list)->shared_count : 0;
} while (read_seqcount_retry(&obj->seq, seq));
smp_rmb(); /* See reservation_object_add_excl_fence */This fence here I think prevents the re-reading of ->fence_excl from getting hoisted above the critical reads. So this is just the open-coded seqlock retry loop.
Without the seq.
The dilemma for dropping the seq would be what if we were to add another state here, such as modified or even invalidate.
} while (rcu_access_pointer(obj->fence_excl) != *excl);What if someone is real fast (like really real fast) and recycles the exclusive fence so you read the same pointer twice, but everything else changed? reused fence pointer is a lot more likely than seqlock wrapping around.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
I'm not worried about the fence, that part is fine. But we're defacto using the fence as a fancy seqlock-of-sorts. And if the fence gets reused and the pointers match, then our seqlock-of-sorts breaks. But I haven't looked around whether there's more in the code that makes this an irrelevant issue. -Daniel
Quoting Daniel Vetter (2019-08-14 18:06:26)
On Wed, Aug 14, 2019 at 05:42:48PM +0100, Chris Wilson wrote:
Quoting Daniel Vetter (2019-08-14 16:39:08)
[snip]
if (old)
old->shared_count = 0;write_seqcount_end(&obj->seq);
smp_store_mb(old->shared_count, 0);So your comment and the kerneldoc don't match up. Quoting Documentation/memory-barriers.txt:
This assigns the value to the variable and then inserts a full memory barrier after it. It isn't guaranteed to insert anything more than a compiler barrier in a UP compilation.So order is 1. store 2. fence, but your comment suggests you want it the other way round.
What's more weird is that it is a fully serialising instruction that is used to fence first as part of the update. If that's way PeterZ uses it...
I haven't looked at the implementations tbh, just going with the text. Or do you mean in the write_seqlock that we're replacing?
Nah, I misremembered set_current_state(), all that implies is the fence is before the following instructions. I have some recollection that it can be used as a RELEASE operation (if only because it is a locked xchg). If all else fails, make it an xchg_release(). Or normal assignment + smp_wmb().
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
I'm not worried about the fence, that part is fine. But we're defacto using the fence as a fancy seqlock-of-sorts. And if the fence gets reused and the pointers match, then our seqlock-of-sorts breaks. But I haven't looked around whether there's more in the code that makes this an irrelevant issue.
No, it should not break if we replace the fence with the same pointer. If the fence pointer expires, reused and assigned back as the excl_fence -- it is still the excl_fence and by the properties of that excl_fence construction, it is later than the shared_fences. -Chris
On Wed, Aug 14, 2019 at 06:20:28PM +0100, Chris Wilson wrote:
Quoting Daniel Vetter (2019-08-14 18:06:26)
On Wed, Aug 14, 2019 at 05:42:48PM +0100, Chris Wilson wrote:
Quoting Daniel Vetter (2019-08-14 16:39:08)
[snip]
> if (old) > - old->shared_count = 0; > - write_seqcount_end(&obj->seq); > + smp_store_mb(old->shared_count, 0);
So your comment and the kerneldoc don't match up. Quoting Documentation/memory-barriers.txt:
This assigns the value to the variable and then inserts a full memory barrier after it. It isn't guaranteed to insert anything more than a compiler barrier in a UP compilation.So order is 1. store 2. fence, but your comment suggests you want it the other way round.
What's more weird is that it is a fully serialising instruction that is used to fence first as part of the update. If that's way PeterZ uses it...
I haven't looked at the implementations tbh, just going with the text. Or do you mean in the write_seqlock that we're replacing?
Nah, I misremembered set_current_state(), all that implies is the fence is before the following instructions. I have some recollection that it can be used as a RELEASE operation (if only because it is a locked xchg). If all else fails, make it an xchg_release(). Or normal assignment + smp_wmb().
Yeah that one is called smp_store_release, not smp_store_mb. I think smp_store_release is the right one here.
It's an exclusive fence. If it is replaced, it must be later than all the shared fences (and dependent on them directly or indirectly), and so still a consistent snapshot.
I'm not worried about the fence, that part is fine. But we're defacto using the fence as a fancy seqlock-of-sorts. And if the fence gets reused and the pointers match, then our seqlock-of-sorts breaks. But I haven't looked around whether there's more in the code that makes this an irrelevant issue.
No, it should not break if we replace the fence with the same pointer. If the fence pointer expires, reused and assigned back as the excl_fence -- it is still the excl_fence and by the properties of that excl_fence construction, it is later than the shared_fences.
So I thought the rules are that if we have an exclusive fence and shared fences both present, then the shared fences are after the exclusive one.
But if we race here, then I think we could accidentally sample shared fences from _before_ the exclusive fences. Rough timeline:
exlusive fence 1 -> shared fence 2 -> exclusive fence, but reuses memory of fence 1
Then we sample fence 1, capture the shared fence 2, and notice that the exclusive fence pointer is the same (but not the fence on the timeline) and conclude that we got a consistent sample.
But the only consistent sample with the new fence state would be only the exclusive fence.
Reminds me I forgot to look for the usual kref_get_unless_zero trickery we also need to do here to make sure we have the right fence. -Daniel
Quoting Christian König (2019-08-07 14:53:09)
Add a new helper to get a consistent set of pointers from the reservation object. While at it group all access helpers together in the header file.
v2: correctly return shared_count as well
Signed-off-by: Christian König christian.koenig@amd.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
linaro-mm-sig@lists.linaro.org
-
Chris Wilson -
Christian König -
Daniel Vetter -
Koenig, Christian