Re: [PATCH 4/4] dma-buf: nuke reservation_object seq number

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel