| Message ID | 20241113134838.52608-1-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/amdgpu: Make the submission path memory reclaim safe | expand |
Am 13.11.24 um 14:48 schrieb Tvrtko Ursulin: > From: Tvrtko Ursulin <tvrtko.ursulin@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. Do we have system wide workqueues for that? It seems a bit overkill that amdgpu has to allocate one on his own. Apart from that looks good to me. Regards, Christian. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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@vger.kernel.org > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Philipp Stanner <pstanner@redhat.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Christian König <christian.koenig@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 {
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@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@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@vger.kernel.org >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: Danilo Krummrich <dakr@kernel.org> >> Cc: Philipp Stanner <pstanner@redhat.com> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Christian König <christian.koenig@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 { >
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 {