Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7

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

Am 04.10.21 um 12:50 schrieb Tvrtko Ursulin:
>
> On 04/10/2021 11:44, Christian König wrote:
>> Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin:
>>>
>>> On 04/10/2021 10:53, Christian König wrote:
>>>> Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
>>>>>
>>>>> On 01/10/2021 11:05, Christian König wrote:
>>>>>> Abstract the complexity of iterating over all the fences
>>>>>> in a dma_resv object.
>>>>>>
>>>>>> The new loop handles the whole RCU and retry dance and
>>>>>> returns only fences where we can be sure we grabbed the
>>>>>> right one.
>>>>>>
>>>>>> v2: fix accessing the shared fences while they might be freed,
>>>>>>      improve kerneldoc, rename _cursor to _iter, add
>>>>>>      dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
>>>>>>
>>>>>> v3: restructor the code, move rcu_read_lock()/unlock() into the
>>>>>>      iterator, add dma_resv_iter_is_restarted()
>>>>>>
>>>>>> v4: fix NULL deref when no explicit fence exists, drop superflous
>>>>>>      rcu_read_lock()/unlock() calls.
>>>>>>
>>>>>> v5: fix typos in the documentation
>>>>>>
>>>>>> v6: fix coding error when excl fence is NULL
>>>>>>
>>>>>> v7: one more logic fix
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>   drivers/dma-buf/dma-resv.c | 100 
>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>   include/linux/dma-resv.h   |  95 
>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>   2 files changed, 195 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>>>>>> index 84fbe60629e3..3cbcf66a137e 100644
>>>>>> --- a/drivers/dma-buf/dma-resv.c
>>>>>> +++ b/drivers/dma-buf/dma-resv.c
>>>>>> @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct 
>>>>>> dma_resv *obj, struct dma_fence *fence)
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(dma_resv_add_excl_fence);
>>>>>>   +/**
>>>>>> + * dma_resv_iter_restart_unlocked - restart the unlocked iterator
>>>>>> + * @cursor: The dma_resv_iter object to restart
>>>>>> + *
>>>>>> + * Restart the unlocked iteration by initializing the cursor 
>>>>>> object.
>>>>>> + */
>>>>>> +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter 
>>>>>> *cursor)
>>>>>> +{
>>>>>> +    cursor->seq = read_seqcount_begin(&cursor->obj->seq);
>>>>>> +    cursor->index = -1;
>>>>>> +    if (cursor->all_fences)
>>>>>> +        cursor->fences = dma_resv_shared_list(cursor->obj);
>>>>>> +    else
>>>>>> +        cursor->fences = NULL;
>>>>>> +    cursor->is_restarted = true;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
>>>>>> + * @cursor: cursor to record the current position
>>>>>> + *
>>>>>> + * Return all the fences in the dma_resv object which are not 
>>>>>> yet signaled.
>>>>>> + * The returned fence has an extra local reference so will stay 
>>>>>> alive.
>>>>>> + * If a concurrent modify is detected the whole iteration is 
>>>>>> started over again.
>>>>>> + */
>>>>>> +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter 
>>>>>> *cursor)
>>>>>> +{
>>>>>> +    struct dma_resv *obj = cursor->obj;
>>>>>> +
>>>>>> +    do {
>>>>>> +        /* Drop the reference from the previous round */
>>>>>> +        dma_fence_put(cursor->fence);
>>>>>> +
>>>>>> +        if (cursor->index == -1) {
>>>>>> +            cursor->fence = dma_resv_excl_fence(obj);
>>>>>> +            cursor->index++;
>>>>>> +            if (!cursor->fence)
>>>>>> +                continue;
>>>>>> +
>>>>>> +        } else if (!cursor->fences ||
>>>>>> +               cursor->index >= cursor->fences->shared_count) {
>>>>>> +            cursor->fence = NULL;
>>>>>> +            break;
>>>>>> +
>>>>>> +        } else {
>>>>>> +            struct dma_resv_list *fences = cursor->fences;
>>>>>> +            unsigned int idx = cursor->index++;
>>>>>> +
>>>>>> +            cursor->fence = rcu_dereference(fences->shared[idx]);
>>>>>> +        }
>>>>>> +        cursor->fence = dma_fence_get_rcu(cursor->fence);
>>>>>
>>>>> Worth having an assert dma_fence_get_rcu does not fail here? Not 
>>>>> sure that I have seen debug build only asserts though on the DRM 
>>>>> core side.
>>>>
>>>> That won't work. It's perfectly valid for dma_fence_get_rcu() to 
>>>> return NULL when we are racing here. Keep in mind that we don't 
>>>> hold any locks.
>>>
>>> Ah yes.. No need to change anything then, sorry for the confusion. I 
>>> did not find any holes, the rest was just about how to maybe make 
>>> the flow more obvious. Let me know if you want r-b now or later.
>>
>> Now would be good. I've tried to make that more cleaner, but this 
>> only lead to repeating the code more often.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks, but what about the rest?

The selftests in this version still have some bugs which I already 
fixed, but I think we could push most of the set.

Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>> What we could do is to return NULL and repeat with a new sequence 
>>>> immediately though.
>>>>
>>>>>
>>>>> On the bike shedding front, would it be clearer if the continue 
>>>>> condition on signaled fences was standalone, using the continue 
>>>>> statement? I'd also possibly re-arrange the three if-else blocks 
>>>>> so that the end of iteration is not sandwiched between blocks 
>>>>> handling exclusive and shared, and flow tweaked a bit, like:
>>>>>
>>>>>   struct dma_fence *fence = cursor->fence;
>>>>>   int index = cursor->index;
>>>>>
>>>>>   dma_fence_put(fence);
>>>>>   fence = NULL;
>>>>>
>>>>> next:
>>>>>   if (index == -1) {
>>>>>     /* Try picking the exclusive fence. */
>>>>>     index++;
>>>>>     fence = dma_resv_excl_fence(obj);
>>>>>     if (!fence)
>>>>>         goto next;
>>>>>   } else if (cursor->fences && index < 
>>>>> cursor->fences->shared_count) {
>>>>>       /* Try picking next shared fence. */
>>>>>     struct dma_resv_list *fences = cursor->fences;
>>>>>
>>>>>     fence = rcu_dereference(fences->shared[index++]);
>>>>>   }
>>>>>
>>>>>   if (fence) {
>>>>>       if (dma_fence_is_signaled(fence))
>>>>>         goto next; /* Skip signaled. */
>>>>>
>>>>>     fence = dma_fence_get_rcu(fence);
>>>>>     WARN_ON(!fence);
>>>>> }
>>>>>
>>>>>   cursor->fence = fence;
>>>>>   cursor->index = index;
>>>>>
>>>>> (I started with a loop here but ended with goto based flow since 
>>>>> it ended up more succinct.)
>>>>>
>>>>> At least if I don't have a handling flaw in there it looks like 
>>>>> easier to follow flow to me. Plus picking a not signaled fence 
>>>>> works without a reference FWIW.
>>>>
>>>> I strongly don't think that this will work correctly. You need to 
>>>> grab a reference first when you want to call 
>>>> dma_fence_is_signaled(), that's why I used the testbit approach 
>>>> initially.
>>>>
>>>>> How does it look to you?
>>>>
>>>> Mhm, let me try to reorder the loop once more.
>>>>
>>>> Thanks,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>> +        if (!cursor->fence || 
>>>>>> !dma_fence_is_signaled(cursor->fence))
>>>>>> +            break;
>>>>>> +    } while (true);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_first_unlocked - first fence in an unlocked 
>>>>>> dma_resv obj.
>>>>>> + * @cursor: the cursor with the current position
>>>>>> + *
>>>>>> + * Returns the first fence from an unlocked dma_resv obj.
>>>>>> + */
>>>>>> +struct dma_fence *dma_resv_iter_first_unlocked(struct 
>>>>>> dma_resv_iter *cursor)
>>>>>> +{
>>>>>> +    rcu_read_lock();
>>>>>> +    do {
>>>>>> +        dma_resv_iter_restart_unlocked(cursor);
>>>>>> +        dma_resv_iter_walk_unlocked(cursor);
>>>>>> +    } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq));
>>>>>> +    rcu_read_unlock();
>>>>>> +
>>>>>> +    return cursor->fence;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(dma_resv_iter_first_unlocked);
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_next_unlocked - next fence in an unlocked 
>>>>>> dma_resv obj.
>>>>>> + * @cursor: the cursor with the current position
>>>>>> + *
>>>>>> + * Returns the next fence from an unlocked dma_resv obj.
>>>>>> + */
>>>>>> +struct dma_fence *dma_resv_iter_next_unlocked(struct 
>>>>>> dma_resv_iter *cursor)
>>>>>> +{
>>>>>> +    bool restart;
>>>>>> +
>>>>>> +    rcu_read_lock();
>>>>>> +    cursor->is_restarted = false;
>>>>>> +    restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq);
>>>>>> +    do {
>>>>>> +        if (restart)
>>>>>> +            dma_resv_iter_restart_unlocked(cursor);
>>>>>> +        dma_resv_iter_walk_unlocked(cursor);
>>>>>> +        restart = true;
>>>>>> +    } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq));
>>>>>> +    rcu_read_unlock();
>>>>>> +
>>>>>> +    return cursor->fence;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(dma_resv_iter_next_unlocked);
>>>>>> +
>>>>>>   /**
>>>>>>    * dma_resv_copy_fences - Copy all fences from src to dst.
>>>>>>    * @dst: the destination reservation object
>>>>>> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
>>>>>> index 9100dd3dc21f..5d7d28cb9008 100644
>>>>>> --- a/include/linux/dma-resv.h
>>>>>> +++ b/include/linux/dma-resv.h
>>>>>> @@ -149,6 +149,101 @@ struct dma_resv {
>>>>>>       struct dma_resv_list __rcu *fence;
>>>>>>   };
>>>>>>   +/**
>>>>>> + * struct dma_resv_iter - current position into the dma_resv fences
>>>>>> + *
>>>>>> + * Don't touch this directly in the driver, use the accessor 
>>>>>> function instead.
>>>>>> + */
>>>>>> +struct dma_resv_iter {
>>>>>> +    /** @obj: The dma_resv object we iterate over */
>>>>>> +    struct dma_resv *obj;
>>>>>> +
>>>>>> +    /** @all_fences: If all fences should be returned */
>>>>>> +    bool all_fences;
>>>>>> +
>>>>>> +    /** @fence: the currently handled fence */
>>>>>> +    struct dma_fence *fence;
>>>>>> +
>>>>>> +    /** @seq: sequence number to check for modifications */
>>>>>> +    unsigned int seq;
>>>>>> +
>>>>>> +    /** @index: index into the shared fences */
>>>>>> +    unsigned int index;
>>>>>> +
>>>>>> +    /** @fences: the shared fences */
>>>>>> +    struct dma_resv_list *fences;
>>>>>> +
>>>>>> +    /** @is_restarted: true if this is the first returned fence */
>>>>>> +    bool is_restarted;
>>>>>> +};
>>>>>> +
>>>>>> +struct dma_fence *dma_resv_iter_first_unlocked(struct 
>>>>>> dma_resv_iter *cursor);
>>>>>> +struct dma_fence *dma_resv_iter_next_unlocked(struct 
>>>>>> dma_resv_iter *cursor);
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_begin - initialize a dma_resv_iter object
>>>>>> + * @cursor: The dma_resv_iter object to initialize
>>>>>> + * @obj: The dma_resv object which we want to iterate over
>>>>>> + * @all_fences: If all fences should be returned or just the 
>>>>>> exclusive one
>>>>>> + */
>>>>>> +static inline void dma_resv_iter_begin(struct dma_resv_iter 
>>>>>> *cursor,
>>>>>> +                       struct dma_resv *obj,
>>>>>> +                       bool all_fences)
>>>>>> +{
>>>>>> +    cursor->obj = obj;
>>>>>> +    cursor->all_fences = all_fences;
>>>>>> +    cursor->fence = NULL;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_end - cleanup a dma_resv_iter object
>>>>>> + * @cursor: the dma_resv_iter object which should be cleaned up
>>>>>> + *
>>>>>> + * Make sure that the reference to the fence in the cursor is 
>>>>>> properly
>>>>>> + * dropped.
>>>>>> + */
>>>>>> +static inline void dma_resv_iter_end(struct dma_resv_iter *cursor)
>>>>>> +{
>>>>>> +    dma_fence_put(cursor->fence);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_is_exclusive - test if the current fence is the 
>>>>>> exclusive one
>>>>>> + * @cursor: the cursor of the current position
>>>>>> + *
>>>>>> + * Returns true if the currently returned fence is the exclusive 
>>>>>> one.
>>>>>> + */
>>>>>> +static inline bool dma_resv_iter_is_exclusive(struct 
>>>>>> dma_resv_iter *cursor)
>>>>>> +{
>>>>>> +    return cursor->index == -1;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_iter_is_restarted - test if this is the first fence 
>>>>>> after a restart
>>>>>> + * @cursor: the cursor with the current position
>>>>>> + *
>>>>>> + * Return true if this is the first fence in an iteration after 
>>>>>> a restart.
>>>>>> + */
>>>>>> +static inline bool dma_resv_iter_is_restarted(struct 
>>>>>> dma_resv_iter *cursor)
>>>>>> +{
>>>>>> +    return cursor->is_restarted;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * dma_resv_for_each_fence_unlocked - unlocked fence iterator
>>>>>> + * @cursor: a struct dma_resv_iter pointer
>>>>>> + * @fence: the current fence
>>>>>> + *
>>>>>> + * Iterate over the fences in a struct dma_resv object without 
>>>>>> holding the
>>>>>> + * &dma_resv.lock and using RCU instead. The cursor needs to be 
>>>>>> initialized
>>>>>> + * with dma_resv_iter_begin() and cleaned up with 
>>>>>> dma_resv_iter_end(). Inside
>>>>>> + * the iterator a reference to the dma_fence is held and the RCU 
>>>>>> lock dropped.
>>>>>> + * When the dma_resv is modified the iteration starts over again.
>>>>>> + */
>>>>>> +#define dma_resv_for_each_fence_unlocked(cursor, 
>>>>>> fence)            \
>>>>>> +    for (fence = dma_resv_iter_first_unlocked(cursor);        \
>>>>>> +         fence; fence = dma_resv_iter_next_unlocked(cursor))
>>>>>> +
>>>>>>   #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base)
>>>>>>   #define dma_resv_assert_held(obj) 
>>>>>> lockdep_assert_held(&(obj)->lock.base)
>>>>>>
>>>>
>>