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

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

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.

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.

> 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