[PATCH] drm/amdgpu: Make the submission path memory reclaim safe

[Tvrtko Ursulin]

On 13/11/2024 14:26, Christian König wrote:
> Am 13.11.24 um 14:48 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>
>> As commit 746ae46c1113 ("drm/sched: Mark scheduler work queues with 
>> WQ_MEM_RECLAIM")
>> points out, ever since
>> a6149f039369 ("drm/sched: Convert drm scheduler to use a work queue 
>> rather than kthread"),
>> any workqueue flushing done from the job submission path must only
>> involve memory reclaim safe workqueues to be safe against reclaim
>> deadlocks.
>>
>> This is also pointed out by workqueue sanity checks:
>>
>>   [ ] workqueue: WQ_MEM_RECLAIM sdma0:drm_sched_run_job_work 
>> [gpu_sched] is flushing !WQ_MEM_RECLAIM 
>> events:amdgpu_device_delay_enable_gfx_off [amdgpu]
>> ...
>>   [ ] Workqueue: sdma0 drm_sched_run_job_work [gpu_sched]
>> ...
>>   [ ] Call Trace:
>>   [ ]  <TASK>
>> ...
>>   [ ]  ? check_flush_dependency+0xf5/0x110
>> ...
>>   [ ]  cancel_delayed_work_sync+0x6e/0x80
>>   [ ]  amdgpu_gfx_off_ctrl+0xab/0x140 [amdgpu]
>>   [ ]  amdgpu_ring_alloc+0x40/0x50 [amdgpu]
>>   [ ]  amdgpu_ib_schedule+0xf4/0x810 [amdgpu]
>>   [ ]  ? drm_sched_run_job_work+0x22c/0x430 [gpu_sched]
>>   [ ]  amdgpu_job_run+0xaa/0x1f0 [amdgpu]
>>   [ ]  drm_sched_run_job_work+0x257/0x430 [gpu_sched]
>>   [ ]  process_one_work+0x217/0x720
>> ...
>>   [ ]  </TASK>
>>
>> Fix this by creating a memory reclaim safe driver workqueue and make the
>> submission path use it.
> 
> Oh well, that is a really good catch! I wasn't aware the workqueues 
> could be blocked by memory reclaim as well.

Only credit I can take is for the habit that I often run with many 
kernel debugging aids enabled.

> Do we have system wide workqueues for that? It seems a bit overkill that 
> amdgpu has to allocate one on his own.

I wondered the same but did not find any. Only ones I am aware of are 
system_wq&co created in workqueue_init_early().

Regards,

Tvrtko

> Apart from that looks good to me.
> 
> Regards,
> Christian.
> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>> References: 746ae46c1113 ("drm/sched: Mark scheduler work queues with 
>> WQ_MEM_RECLAIM")
>> Fixes: a6149f039369 ("drm/sched: Convert drm scheduler to use a work 
>> queue rather than kthread")
>> Cc: stable at vger.kernel.org
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Danilo Krummrich <dakr at kernel.org>
>> Cc: Philipp Stanner <pstanner at redhat.com>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 25 +++++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |  5 +++--
>>   3 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 7645e498faa4..a6aad687537e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -268,6 +268,8 @@ extern int amdgpu_agp;
>>   extern int amdgpu_wbrf;
>> +extern struct workqueue_struct *amdgpu_reclaim_wq;
>> +
>>   #define AMDGPU_VM_MAX_NUM_CTX            4096
>>   #define AMDGPU_SG_THRESHOLD            (256*1024*1024)
>>   #define AMDGPU_WAIT_IDLE_TIMEOUT_IN_MS            3000
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 38686203bea6..f5b7172e8042 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -255,6 +255,8 @@ struct amdgpu_watchdog_timer amdgpu_watchdog_timer 
>> = {
>>       .period = 0x0, /* default to 0x0 (timeout disable) */
>>   };
>> +struct workqueue_struct *amdgpu_reclaim_wq;
>> +
>>   /**
>>    * DOC: vramlimit (int)
>>    * Restrict the total amount of VRAM in MiB for testing.  The 
>> default is 0 (Use full VRAM).
>> @@ -2971,6 +2973,21 @@ static struct pci_driver amdgpu_kms_pci_driver = {
>>       .dev_groups = amdgpu_sysfs_groups,
>>   };
>> +static int amdgpu_wq_init(void)
>> +{
>> +    amdgpu_reclaim_wq =
>> +        alloc_workqueue("amdgpu-reclaim", WQ_MEM_RECLAIM, 0);
>> +    if (!amdgpu_reclaim_wq)
>> +        return -ENOMEM;
>> +
>> +    return 0;
>> +}
>> +
>> +static void amdgpu_wq_fini(void)
>> +{
>> +    destroy_workqueue(amdgpu_reclaim_wq);
>> +}
>> +
>>   static int __init amdgpu_init(void)
>>   {
>>       int r;
>> @@ -2978,6 +2995,10 @@ static int __init amdgpu_init(void)
>>       if (drm_firmware_drivers_only())
>>           return -EINVAL;
>> +    r = amdgpu_wq_init();
>> +    if (r)
>> +        goto error_wq;
>> +
>>       r = amdgpu_sync_init();
>>       if (r)
>>           goto error_sync;
>> @@ -3006,6 +3027,9 @@ static int __init amdgpu_init(void)
>>       amdgpu_sync_fini();
>>   error_sync:
>> +    amdgpu_wq_fini();
>> +
>> +error_wq:
>>       return r;
>>   }
>> @@ -3017,6 +3041,7 @@ static void __exit amdgpu_exit(void)
>>       amdgpu_acpi_release();
>>       amdgpu_sync_fini();
>>       amdgpu_fence_slab_fini();
>> +    amdgpu_wq_fini();
>>       mmu_notifier_synchronize();
>>       amdgpu_xcp_drv_release();
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index 2f3f09dfb1fd..f8fd71d9382f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -790,8 +790,9 @@ void amdgpu_gfx_off_ctrl(struct amdgpu_device 
>> *adev, bool enable)
>>                           AMD_IP_BLOCK_TYPE_GFX, true))
>>                       adev->gfx.gfx_off_state = true;
>>               } else {
>> -                schedule_delayed_work(&adev->gfx.gfx_off_delay_work,
>> -                          delay);
>> +                queue_delayed_work(amdgpu_reclaim_wq,
>> +                           &adev->gfx.gfx_off_delay_work,
>> +                           delay);
>>               }
>>           }
>>       } else {
>