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.
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
