[PATCH] drm/amdgpu: fix amdgpu_amdkfd_remove_eviction_fence

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Am 15.08.2018 um 20:17 schrieb Felix Kuehling:
> On 2018-08-15 03:02 AM, Christian König wrote:
>> Hi Felix,
>>
>> yeah, you pretty much nailed it.
>>
>> The problem is that the array itself is RCU protected. This means that
>> you can only copy the whole structure when you want to update it.
>>
>> The exception is reservation_object_add_shared() which only works
>> because we replace an either signaled fence or a fence within the same
>> context but a later sequence number.
>>
>> This also explains why this is only a band aid and the whole approach
>> of removing fences doesn't work in general. At any time somebody could
>> have taken an RCU reference to the old array, created a copy of it and
>> is now still using this one.
>>
>> The only 100% correct solution would be to mark the existing fence as
>> signaled and replace it everywhere else.
> Depends what you're trying to achieve. I think the problem you see is,
> that some reader may still be using the old reservation_object_list copy
> with the fence still in it. But, the remaining lifetime of the
> reservation_object_list copy is limited. If we wanted to be sure no more
> copies with the old fence exist, all we'd need to do is call
> synchronize_rcu. Maybe we need to do that before releasing the fence
> references, or release the fence reference in an RCU callback to be safe.

The assumption that the fence would die with the array is what is 
incorrect here.

The lifetime of RCUed array object is limit, but there is absolutely no 
guarantee that somebody didn't made a copy of the fences.

E.g. somebody could have called reservation_object_get_fences_rcu(), 
reservation_object_copy_fences() or a concurrent 
reservation_object_wait_timeout_rcu() is underway.

That's also the reason why fences live for much longer than their 
signaling, e.g. somebody can have a reference to the fence object even 
hours after it is signaled.

Regards,
Christian.

>
> Regards,
>    Felix
>
>> Going to fix the copy&paste error I made with the sequence number and
>> send it out again.
>>
>> Regards,
>> Christian.
>>
>> Am 14.08.2018 um 22:17 schrieb Felix Kuehling:
>>> [+Harish]
>>>
>>> I think this looks good for the most part. See one comment inline below.
>>>
>>> But bear with me while I'm trying to understand what was wrong with the
>>> old code. Please correct me if I get this wrong or point out anything
>>> I'm missing.
>>>
>>> The reservation_object_list looks to be protected by a combination of
>>> three mechanism:
>>>
>>>     * Holding the reservation object
>>>     * RCU
>>>     * seqcount
>>>
>>> Updating the fence list requires holding the reservation object. But
>>> there are some readers that can be called without holding that lock
>>> (reservation_object_copy_fences, reservation_object_get_fences_rcu,
>>> reservation_object_wait_timeout_rcu,
>>> reservation_object_test_signaled_rcu). They rely on RCU to work on a
>>> copy and seqcount to make sure they had the most up-to-date information.
>>> So any function updating the fence lists needs to do RCU and seqcount
>>> correctly to prevent breaking those readers.
>>>
>>> As I understand it, RCU with seqcount retry just means that readers will
>>> spin retrying while there are writers, and writers are never blocked by
>>> readers. Writers are blocked by each other, because they need to hold
>>> the reservation.
>>>
>>> The code you added looks a lot like
>>> reservation_object_add_shared_replace, which removes fences that have
>>> signalled, and atomically replaces obj->fences with a new
>>> reservation_fence_list. That atomicity is important because each pointer
>>> in the obj->fences->shared array is separately protected by RCU, but not
>>> the array as a whole or the shared_count.
>>>
>>> See one comment inline.
>>>
>>> Regards,
>>>     Felix
>>>
>>> On 2018-08-14 03:39 AM, Christian König wrote:
>>>> Fix quite a number of bugs here. Unfortunately only compile tested.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 103
>>>> ++++++++++-------------
>>>>    1 file changed, 46 insertions(+), 57 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> index fa38a960ce00..dece31516dc4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> @@ -206,11 +206,9 @@ static int
>>>> amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo,
>>>>                        struct amdgpu_amdkfd_fence ***ef_list,
>>>>                        unsigned int *ef_count)
>>>>    {
>>>> -    struct reservation_object_list *fobj;
>>>> -    struct reservation_object *resv;
>>>> -    unsigned int i = 0, j = 0, k = 0, shared_count;
>>>> -    unsigned int count = 0;
>>>> -    struct amdgpu_amdkfd_fence **fence_list;
>>>> +    struct reservation_object *resv = bo->tbo.resv;
>>>> +    struct reservation_object_list *old, *new;
>>>> +    unsigned int i, j, k;
>>>>          if (!ef && !ef_list)
>>>>            return -EINVAL;
>>>> @@ -220,76 +218,67 @@ static int
>>>> amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo,
>>>>            *ef_count = 0;
>>>>        }
>>>>    -    resv = bo->tbo.resv;
>>>> -    fobj = reservation_object_get_list(resv);
>>>> -
>>>> -    if (!fobj)
>>>> +    old = reservation_object_get_list(resv);
>>>> +    if (!old)
>>>>            return 0;
>>>>    -    preempt_disable();
>>>> -    write_seqcount_begin(&resv->seq);
>>>> +    new = kmalloc(offsetof(typeof(*new), shared[old->shared_max]),
>>>> +              GFP_KERNEL);
>>>> +    if (!new)
>>>> +        return -ENOMEM;
>>>>    -    /* Go through all the shared fences in the resevation object. If
>>>> -     * ef is specified and it exists in the list, remove it and
>>>> reduce the
>>>> -     * count. If ef is not specified, then get the count of
>>>> eviction fences
>>>> -     * present.
>>>> +    /* Go through all the shared fences in the resevation object
>>>> and sort
>>>> +     * the interesting ones to the end of the list.
>>>>         */
>>>> -    shared_count = fobj->shared_count;
>>>> -    for (i = 0; i < shared_count; ++i) {
>>>> +    for (i = 0, j = old->shared_count, k = 0; i <
>>>> old->shared_count; ++i) {
>>>>            struct dma_fence *f;
>>>>    -        f = rcu_dereference_protected(fobj->shared[i],
>>>> +        f = rcu_dereference_protected(old->shared[i],
>>>>                              reservation_object_held(resv));
>>>>    -        if (ef) {
>>>> -            if (f->context == ef->base.context) {
>>>> -                dma_fence_put(f);
>>>> -                fobj->shared_count--;
>>>> -            } else {
>>>> -                RCU_INIT_POINTER(fobj->shared[j++], f);
>>>> -            }
>>>> -        } else if (to_amdgpu_amdkfd_fence(f))
>>>> -            count++;
>>>> +        if ((ef && f->context == ef->base.context) ||
>>>> +            (!ef && to_amdgpu_amdkfd_fence(f)))
>>>> +            RCU_INIT_POINTER(new->shared[--j], f);
>>>> +        else
>>>> +            RCU_INIT_POINTER(new->shared[k++], f);
>>>>        }
>>>> -    write_seqcount_end(&resv->seq);
>>>> -    preempt_enable();
>>>> +    new->shared_max = old->shared_max;
>>>> +    new->shared_count = k;
>>>>    -    if (ef || !count)
>>>> -        return 0;
>>>> +    if (!ef) {
>>>> +        unsigned int count = old->shared_count - j;
>>>>    -    /* Alloc memory for count number of eviction fence pointers.
>>>> Fill the
>>>> -     * ef_list array and ef_count
>>>> -     */
>>>> -    fence_list = kcalloc(count, sizeof(struct amdgpu_amdkfd_fence *),
>>>> -                 GFP_KERNEL);
>>>> -    if (!fence_list)
>>>> -        return -ENOMEM;
>>>> +        /* Alloc memory for count number of eviction fence
>>>> pointers. Fill the
>>>> +         * ef_list array and ef_count
>>>> +         */
>>>> +        *ef_list = kcalloc(count, sizeof(**ef_list), GFP_KERNEL);
>>>> +        *ef_count = count;
>>>>    +        if (!*ef_list) {
>>>> +            kfree(new);
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /* Install the new fence list, seqcount provides the barriers */
>>>> +    preempt_disable();
>>>> +    write_seqcount_begin(&resv->seq);
>>>> +    RCU_INIT_POINTER(resv->fence, new);
>>>>        preempt_disable();
>>>>        write_seqcount_begin(&resv->seq);
>>> You're disabling preemption and calling write_seqcount_begin twice. I
>>> think this is meant to be
>>>
>>>      write_seqcount_end(&resv->seq);
>>>      preempt_enable();
>>>
>>>
>>>>    -    j = 0;
>>>> -    for (i = 0; i < shared_count; ++i) {
>>>> +    /* Drop the references to the removed fences or move them to
>>>> ef_list */
>>>> +    for (i = j, k = 0; i < old->shared_count; ++i) {
>>>>            struct dma_fence *f;
>>>> -        struct amdgpu_amdkfd_fence *efence;
>>>>    -        f = rcu_dereference_protected(fobj->shared[i],
>>>> -            reservation_object_held(resv));
>>>> -
>>>> -        efence = to_amdgpu_amdkfd_fence(f);
>>>> -        if (efence) {
>>>> -            fence_list[k++] = efence;
>>>> -            fobj->shared_count--;
>>>> -        } else {
>>>> -            RCU_INIT_POINTER(fobj->shared[j++], f);
>>>> -        }
>>>> +        f = rcu_dereference_protected(new->shared[i],
>>>> +                          reservation_object_held(resv));
>>>> +        if (!ef)
>>>> +            (*ef_list)[k++] = to_amdgpu_amdkfd_fence(f);
>>>> +        else
>>>> +            dma_fence_put(f);
>>>>        }
>>>> -
>>>> -    write_seqcount_end(&resv->seq);
>>>> -    preempt_enable();
>>>> -
>>>> -    *ef_list = fence_list;
>>>> -    *ef_count = k;
>>>> +    kfree_rcu(old, rcu);
>>>>          return 0;
>>>>    }
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux