[RFC PATCH] drm/amdkfd: Run restore_workers on freezable WQs

[Christian König]

Am 30.10.23 um 18:56 schrieb Felix Kuehling:
> On 2023-10-30 13:48, Christian König wrote:
>>
>>
>> Am 30.10.23 um 18:38 schrieb Felix Kuehling:
>>> On 2023-10-30 12:16, Christian König wrote:
>>>>>>> @@ -1904,6 +1906,19 @@ kfd_process_gpuid_from_node(struct 
>>>>>>> kfd_process *p, struct kfd_node *node,
>>>>>>>       return -EINVAL;
>>>>>>>   }
>>>>>>>   +static void signal_eviction_fence(struct kfd_process *p)
>>>>>>> +{
>>>>>>> +    spin_lock(&p->ef_lock);
>>>>>>> +    if (!p->ef)
>>>>>>> +        goto unlock_out;
>>>>>>> +    dma_fence_signal(p->ef);
>>>>>>
>>>>>> This needs to grab the internal lock of the eviction fence, I'm 
>>>>>> not sure that has correct ordering with the newly added ef_lock.
>>>>>
>>>>> Hmm, the only thing we could get a circular lock dependency would 
>>>>> be, if we took the p->ef_lock in a fence callback. I don't think 
>>>>> that ever happens, because even the eviction work runs on a worker 
>>>>> thread (exactly to avoid such lock dependency issues).
>>>>>
>>>>> Anyway, I could try to move the fence_signal out of the critical 
>>>>> section. The lock is only there to ensure that exactly one process 
>>>>> signals and frees the fence.
>>>>>
>>>>
>>>> So basically either the eviction worker or the GPU reset could 
>>>> signal this fence.
>>>>
>>>> In that case I think it would be simpler to grab the reset lock in 
>>>> the eviction worker to protect against a concurrent reset.
>>>
>>> Which reset lock? adev->reset_cntl->reset_lock? I only see that lock 
>>> taken in aldebaran_mode2_perform_reset. I don't understand why this 
>>> is in ASIC-specific code. But even so, it's only taken during the 
>>> actual reset (in aldebaran_mode2_perform_reset). I don't think it 
>>> covers the pre-reset code path that signals the eviction fence.
>>
>> No, what I mean is adev->reset_domain->sem. It's hold in write lock 
>> during the reset and you can grab the read side if you need to delay 
>> a reset.
>>
>> But thinking about that a bit more, you actually don't need any of 
>> this. Something like this here should do fine:
>>
>> tmp = dma_fence_get_rcu_safe(&p->ef);
>> dma_fence_signal(tmp);
>> dma_fence_put(tmp);
>
> dma_fence_get_rcu_safe gets a new reference that dma_fence_put drops. 
> It doesn't drop the original reference in p->ef.
>
> I need a way to ensure that exactly one thread frees the original 
> reference in p->ef. Even with RCU, concurrent writers still need a 
> lock or a mutex. So I don't think I can avoid using another lock here 
> because I have potentially two concurrent writers.

Mhm, why would you need that? Isn't p->ef be replaced with a new fence 
when the process is restored?

Or do you go from fence->NULL->fence? If that's the case then yeah the 
lock is probably your only option.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> dma_fences are always RCU protected and can be signaled from multiple 
>> sources by design.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>