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

[Thomas Hellstrom]

On 04/14/2014 09:42 AM, Maarten Lankhorst wrote:
> op 11-04-14 21:35, Thomas Hellstrom schreef:
>> On 04/11/2014 08:09 PM, Maarten Lankhorst wrote:
>>> op 11-04-14 12:11, Thomas Hellstrom schreef:
>>>> 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?
>>>>
>>> No, because shared_count is protected with seqcount. I only allow
>>> appending to the array, so when
>>> shared_count is validated by seqcount it means that the
>>> [0...shared_count) indexes are valid and non-null.
>>> What could happen though is that the fence at a specific index is
>>> updated with another one from the same
>>> context, but that's harmless.
>>>
>> Hmm, doesn't attaching an exclusive fence clear all shared fence
>> pointers from under a reader?
>>
> No, for that reason. It only resets shared_count to 0.

Ah. OK. I guess I didn't read the code carefully enough.

Thanks,
Thomas



>
> ~Maarten