Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu

[Thomas Hellstrom <thellstrom@vmware.com>]

On 04/11/2014 11:24 AM, Maarten Lankhorst wrote:
> op 11-04-14 10:38, Thomas Hellstrom schreef:
>> Hi, Maarten.
>>
>> Here I believe we encounter a lot of locking inconsistencies.
>>
>> First, it seems you're use a number of pointers as RCU pointers without
>> annotating them as such and use the correct rcu
>> macros when assigning those pointers.
>>
>> Some pointers (like the pointers in the shared fence list) are both used
>> as RCU pointers (in dma_buf_poll()) for example,
>> or considered protected by the seqlock
>> (reservation_object_get_fences_rcu()), which I believe is OK, but then
>> the pointers must
>> be assigned using the correct rcu macros. In the memcpy in
>> reservation_object_get_fences_rcu() we might get away with an
>> ugly typecast, but with a verbose comment that the pointers are
>> considered protected by the seqlock at that location.
>>
>> So I've updated (attached) the headers with proper __rcu annotation and
>> locking comments according to how they are being used in the various
>> reading functions.
>> I believe if we want to get rid of this we need to validate those
>> pointers using the seqlock as well.
>> This will generate a lot of sparse warnings in those places needing
>> rcu_dereference()
>> rcu_assign_pointer()
>> rcu_dereference_protected()
>>
>> With this I think we can get rid of all ACCESS_ONCE macros: It's not
>> needed when the rcu_x() macros are used, and
>> it's never needed for the members protected by the seqlock, (provided
>> that the seq is tested). The only place where I think that's
>> *not* the case is at the krealloc in
>> reservation_object_get_fences_rcu().
>>
>> Also I have some more comments in the
>> reservation_object_get_fences_rcu() function below:
> I felt that the barriers needed for rcu were already provided by
> checking the seqcount lock.
> But looking at rcu_dereference makes it seem harmless to add it in
> more places, it handles
> the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of
checking for us. I guess
we can tolerate a couple of extra barriers for that.

>
> We could probably get away with using RCU_INIT_POINTER on the writer
> side,
> because the smp_wmb is already done by arranging seqcount updates
> correctly.

Hmm. yes, probably. At least in the replace function. I think if we do
it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.


Also  I saw in a couple of places where you're checking the shared
pointers, you're not checking for NULL pointers, which I guess may
happen if shared_count and pointers are not in full sync?

Thanks,
/Thomas


>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index d89a98d2c37b..ca6ef0c4b358 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>>
>> +int reservation_object_get_fences_rcu(struct reservation_object *obj,
>> +                      struct fence **pfence_excl,
>> +                      unsigned *pshared_count,
>> +                      struct fence ***pshared)
>> +{
>> +    unsigned shared_count = 0;
>> +    unsigned retry = 1;
>> +    struct fence **shared = NULL, *fence_excl = NULL;
>> +    int ret = 0;
>> +
>> +    while (retry) {
>> +        struct reservation_object_list *fobj;
>> +        unsigned seq, retry;
>> You're shadowing retry?
> Oops.
>>
>>> +
>>> +        seq = read_seqcount_begin(&obj->seq);
>>> +
>>> +        rcu_read_lock();
>>> +
>>> +        fobj = ACCESS_ONCE(obj->fence);
>>> +        if (fobj) {
>>> +            struct fence **nshared;
>>> +
>>> +            shared_count = ACCESS_ONCE(fobj->shared_count);
>>> +            nshared = krealloc(shared, sizeof(*shared) *
>>> shared_count, GFP_KERNEL);
>> krealloc inside rcu_read_lock(). Better to put this first in the loop.
> Except that shared_count isn't known until the rcu_read_lock is taken.
>> Thanks,
>> Thomas
> ~Maarten