[PATCHv2 2/3] drm/amdkfd: Update queue unmap after VM fault with MES

[Felix Kuehling]

On 2024-08-15 17:08, Joshi, Mukul wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Felix,
>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling at amd.com>
>> Sent: Thursday, August 15, 2024 2:25 PM
>> To: Joshi, Mukul <Mukul.Joshi at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Subject: Re: [PATCHv2 2/3] drm/amdkfd: Update queue unmap after VM fault
>> with MES
>>
>> On 2024-08-14 19:27, Mukul Joshi wrote:
>>> MEC FW expects MES to unmap all queues when a VM fault is observed on
>>> a queue and then resumed once the affected process is terminated.
>>> Use the MES Suspend and Resume APIs to achieve this.
>>>
>>> Signed-off-by: Mukul Joshi <mukul.joshi at amd.com>
>>> ---
>>> v1->v2:
>>> - Add MES FW version check.
>>> - Separate out the kfd_dqm_evict_pasid into another function.
>>> - Use amdgpu_mes_suspend/amdgpu_mes_resume to suspend/resume
>> queues.
>>>    .../drm/amd/amdkfd/kfd_device_queue_manager.c | 79
>> ++++++++++++++++++-
>>>    1 file changed, 77 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> index f6e211070299..cb5b866eee3b 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> @@ -319,6 +319,42 @@ static int remove_all_queues_mes(struct
>> device_queue_manager *dqm)
>>>      return retval;
>>>    }
>>>
>>> +static int suspend_all_queues_mes(struct device_queue_manager *dqm) {
>>> +   struct amdgpu_device *adev = (struct amdgpu_device *)dqm->dev-
>>> adev;
>>> +   int r = 0;
>>> +
>>> +   if (dqm->is_hws_hang)
>>> +           return -EIO;
>>> +
>>> +   r = amdgpu_mes_suspend(adev);
>>> +   if (r) {
>>> +           dev_err(adev->dev, "failed to suspend gangs from MES\n");
>>> +           dev_err(adev->dev, "MES might be in unrecoverable state,
>> issue a GPU reset\n");
>>> +           kfd_hws_hang(dqm);
>>> +   }
>>> +
>>> +   return r;
>>> +}
>>> +
>>> +static int resume_all_queues_mes(struct device_queue_manager *dqm) {
>>> +   struct amdgpu_device *adev = (struct amdgpu_device *)dqm->dev-
>>> adev;
>>> +   int r = 0;
>>> +
>>> +   if (dqm->is_hws_hang)
>>> +           return -EIO;
>>> +
>>> +   r = amdgpu_mes_resume(adev);
>>> +   if (r) {
>>> +           dev_err(adev->dev, "failed to resume gangs from MES\n");
>>> +           dev_err(adev->dev, "MES might be in unrecoverable state,
>> issue a GPU reset\n");
>>> +           kfd_hws_hang(dqm);
>>> +   }
>>> +
>>> +   return r;
>>> +}
>>> +
>>>    static void increment_queue_count(struct device_queue_manager *dqm,
>>>                                struct qcm_process_device *qpd,
>>>                                struct queue *q)
>>> @@ -2835,6 +2871,40 @@ void device_queue_manager_uninit(struct
>> device_queue_manager *dqm)
>>>      kfree(dqm);
>>>    }
>>>
>>> +static int kfd_dqm_evict_pasid_mes(struct device_queue_manager *dqm,
>>> +                              struct qcm_process_device *qpd) {
>>> +   struct device *dev = dqm->dev->adev->dev;
>>> +   int ret = 0;
>>> +
>>> +   /* Check if process is already evicted */
>>> +   dqm_lock(dqm);
>>> +   if (qpd->evicted) {
>>> +           dqm_unlock(dqm);
>>> +           goto out;
>> qpd->evicted is a reference count. Without this shortcut,
>> dqm->ops.evict_process_queues will increment the ref count. You probably
>> need to increment it here before dropping the lock. Otherwise two things can
>> go wrong:
>>
>>   1. The corresponding dqm->ops.restore_process_queues will underflow the
>>      reference count
>>   2. A race condition where the queues get restored too early
>>
> The intent here is to check if the process queues are already evicted or not. If its not, then we want
> to suspend all queues, evict all queues (which also updates the evicted refcount) of the affected process,
> and resume all queues.
> If I increment the refcount here, then dqm->ops.evict_process_queues will not evict the queues unless we
> change that function.
> And this function would be called only for the VM fault case, so the process is going to be terminated. Is it
> possible to have dqm->ops.restore_process_queues called on it? Even if it called, I don't think we can have
> underflow of the refcount with the current code.
>
> Can you please explain the case where the dqm->ops.restore_process_queues can cause an underflow
> with the current code? And the scenario for the race?

On GPUs with MES, you pair kfd_dqm_evict_pasid_mes with 
dqm->ops.restore_process_queues. For every call of 
kfd_dqm_evict_pasid_mes there will be a corresponding call of 
dqm->ops.restore_process_queues. If kfd_dqm_evict_pasid_mes doesn't 
increment the qpd->evicted refcount for some cases, the refcount will 
underflow in dqm->ops.restore_process_queues.

Regards,
   Felix


>
>
> Regards,
> Mukul
>
>> Regards,
>>     Felix
>>
>>
>>> +   }
>>> +   dqm_unlock(dqm);
>>> +
>>> +   ret = suspend_all_queues_mes(dqm);
>>> +   if (ret) {
>>> +           dev_err(dev, "Suspending all queues failed");
>>> +           goto out;
>>> +   }
>>> +
>>> +   ret = dqm->ops.evict_process_queues(dqm, qpd);
>>> +   if (ret) {
>>> +           dev_err(dev, "Evicting process queues failed");
>>> +           goto out;
>>> +   }
>>> +
>>> +   ret = resume_all_queues_mes(dqm);
>>> +   if (ret)
>>> +           dev_err(dev, "Resuming all queues failed");
>>> +
>>> +out:
>>> +   return ret;
>>> +}
>>> +
>>>    int kfd_dqm_evict_pasid(struct device_queue_manager *dqm, u32 pasid)
>>>    {
>>>      struct kfd_process_device *pdd;
>>> @@ -2845,8 +2915,13 @@ int kfd_dqm_evict_pasid(struct
>> device_queue_manager *dqm, u32 pasid)
>>>              return -EINVAL;
>>>      WARN(debug_evictions, "Evicting pid %d", p->lead_thread->pid);
>>>      pdd = kfd_get_process_device_data(dqm->dev, p);
>>> -   if (pdd)
>>> -           ret = dqm->ops.evict_process_queues(dqm, &pdd->qpd);
>>> +   if (pdd) {
>>> +           if (dqm->dev->kfd->shared_resources.enable_mes)
>>> +                   ret = kfd_dqm_evict_pasid_mes(dqm, &pdd->qpd);
>>> +           else
>>> +                   ret = dqm->ops.evict_process_queues(dqm, &pdd-
>>> qpd);
>>> +   }
>>> +
>>>      kfd_unref_process(p);
>>>
>>>      return ret;