| Message ID | 20190806150134.104222-8-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] dma-buf: fix busy wait for new shared fences | expand |
Quoting Christian König (2019-08-06 16:01:34) > 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/reservation.c | 20 +++----------------- > include/linux/reservation.h | 9 ++------- > 2 files changed, 5 insertions(+), 24 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 839d72af7ad8..43549a4d6658 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); I think, now has to be rcu_assign_pointer. * Initialize an RCU-protected pointer in special cases where readers * do not need ordering constraints on the CPU or the compiler. These * special cases are: * * 1. This use of RCU_INIT_POINTER() is NULLing out the pointer *or* * 2. The caller has taken whatever steps are required to prevent * RCU readers from concurrently accessing this pointer *or* * 3. The referenced data structure has already been exposed to * readers either at compile time or via rcu_assign_pointer() *and* * * a. You have not made *any* reader-visible changes to * this structure since then *or* * b. It is OK for readers accessing this structure from its * new location to see the old state of the structure. (For * example, the changes were to statistical counters or to * other state where exact synchronization is not required.) We used to apply 2 from the seqcount, and 3 does not apply here. > + /* 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); Hmm. Ok, I think. > { > - unsigned int seq; > - > do { > - seq = read_seqcount_begin(&obj->seq); > *excl = rcu_dereference(obj->fence_excl); > *list = rcu_dereference(obj->fence); > - } while (read_seqcount_retry(&obj->seq, seq)); > + smp_rmb(); /* See reservation_object_add_excl_fence */ > + } while (rcu_access_pointer(obj->fence_excl) != *excl); > } So, if we are add_excl_fence and cancelling a shared-list, before we return we guarantee that the shared-list is set to zero if excl is set as we read. If we add to shared-list during the read, ... Hmm, actually we should return num_list, i.e. do { *list = rcu_dereference(obj->fence); num_list = *list ? (*list)->count : 0; smp_rmb(); } while (...) return num_list. as the relationship between the count and the fence entries is also determined by the mb in add_shared_fence. Oops, that was an oversight in the previous review. > preempt_enable(); > > /* inplace update, no shared fences */ > @@ -370,11 +359,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_assign_pointer again I believe. -Chris
Am 06.08.19 um 21:57 schrieb Chris Wilson: > Quoting Christian König (2019-08-06 16:01:34) >> 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/reservation.c | 20 +++----------------- >> include/linux/reservation.h | 9 ++------- >> 2 files changed, 5 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >> index 839d72af7ad8..43549a4d6658 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); > I think, now has to be rcu_assign_pointer. > > * Initialize an RCU-protected pointer in special cases where readers > * do not need ordering constraints on the CPU or the compiler. These > * special cases are: > * > * 1. This use of RCU_INIT_POINTER() is NULLing out the pointer *or* > * 2. The caller has taken whatever steps are required to prevent > * RCU readers from concurrently accessing this pointer *or* > * 3. The referenced data structure has already been exposed to > * readers either at compile time or via rcu_assign_pointer() *and* > * > * a. You have not made *any* reader-visible changes to > * this structure since then *or* > * b. It is OK for readers accessing this structure from its > * new location to see the old state of the structure. (For > * example, the changes were to statistical counters or to > * other state where exact synchronization is not required.) > > We used to apply 2 from the seqcount, and 3 does not apply here. > >> + /* 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); > Hmm. Ok, I think. > >> { >> - unsigned int seq; >> - >> do { >> - seq = read_seqcount_begin(&obj->seq); >> *excl = rcu_dereference(obj->fence_excl); >> *list = rcu_dereference(obj->fence); >> - } while (read_seqcount_retry(&obj->seq, seq)); >> + smp_rmb(); /* See reservation_object_add_excl_fence */ >> + } while (rcu_access_pointer(obj->fence_excl) != *excl); >> } > So, if we are add_excl_fence and cancelling a shared-list, before we > return we guarantee that the shared-list is set to zero if excl is set > as we read. > > If we add to shared-list during the read, ... Hmm, actually we should > return num_list, i.e. > > do { > *list = rcu_dereference(obj->fence); > num_list = *list ? (*list)->count : 0; > smp_rmb(); > } while (...) > > return num_list. > > as the relationship between the count and the fence entries is also > determined by the mb in add_shared_fence. I've read that multiple times now, but can't follow. Why should we do this? The only important thing is that the readers see the new fence before the increment of the number of fences. When you see this is rather irrelevant. Christian. > > Oops, that was an oversight in the previous review. > >> preempt_enable(); >> >> /* inplace update, no shared fences */ >> @@ -370,11 +359,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_assign_pointer again I believe. > -Chris
Quoting Christian König (2019-08-07 13:08:38) > Am 06.08.19 um 21:57 schrieb Chris Wilson: > > If we add to shared-list during the read, ... Hmm, actually we should > > return num_list, i.e. > > > > do { > > *list = rcu_dereference(obj->fence); > > num_list = *list ? (*list)->count : 0; > > smp_rmb(); > > } while (...) > > > > return num_list. > > > > as the relationship between the count and the fence entries is also > > determined by the mb in add_shared_fence. > > I've read that multiple times now, but can't follow. Why should we do this? > > The only important thing is that the readers see the new fence before > the increment of the number of fences. Exactly. We order the store so that the fence is in the list before we update the count (so that we don't read garbage because the fence isn't there yet). But we don't have the equivalent here for the read once the rmb is removed from the seqcount_read_begin/end looping. We need to see the update in the same order as was stored, and only use the coherent portion of the list. -Chris
Am 07.08.19 um 14:19 schrieb Chris Wilson: > Quoting Christian König (2019-08-07 13:08:38) >> Am 06.08.19 um 21:57 schrieb Chris Wilson: >>> If we add to shared-list during the read, ... Hmm, actually we should >>> return num_list, i.e. >>> >>> do { >>> *list = rcu_dereference(obj->fence); >>> num_list = *list ? (*list)->count : 0; >>> smp_rmb(); >>> } while (...) >>> >>> return num_list. >>> >>> as the relationship between the count and the fence entries is also >>> determined by the mb in add_shared_fence. >> I've read that multiple times now, but can't follow. Why should we do this? >> >> The only important thing is that the readers see the new fence before >> the increment of the number of fences. > Exactly. We order the store so that the fence is in the list before we > update the count (so that we don't read garbage because the fence isn't > there yet). > > But we don't have the equivalent here for the read once the rmb is > removed from the seqcount_read_begin/end looping. We need to see the > update in the same order as was stored, and only use the coherent > portion of the list. Ok that makes sense. Going to fix up the code regarding to that. Christian. > -Chris
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 839d72af7ad8..43549a4d6658 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); + /* 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 */ @@ -370,11 +359,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(); reservation_object_list_free(src_list); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index b8b8273eef00..1dfaf7b1f1da 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; @@ -155,13 +152,11 @@ reservation_object_fences(struct reservation_object *obj, struct dma_fence **excl, struct reservation_object_list **list) { - unsigned int seq; - do { - seq = read_seqcount_begin(&obj->seq); *excl = rcu_dereference(obj->fence_excl); *list = rcu_dereference(obj->fence); - } while (read_seqcount_retry(&obj->seq, seq)); + smp_rmb(); /* See reservation_object_add_excl_fence */ + } while (rcu_access_pointer(obj->fence_excl) != *excl); } /**
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/reservation.c | 20 +++----------------- include/linux/reservation.h | 9 ++------- 2 files changed, 5 insertions(+), 24 deletions(-)