On 15.07.25 12:27, Philipp Stanner wrote: > On Tue, 2025-07-15 at 09:51 +0000, cao, lin wrote: >> >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> >> >> Hi Philipp, >> >> >> Thanks for your review, let me try to clarify why I added drm_sched_wakeup() >> to drm_sched_entity_kill_jobs_work(): >> >> >> When application A submits jobs (a1, a2, a3) and application B submits job >> b1 with a dependency on a1's scheduled fence, the normal execution flow is >> (function drm_sched_run_job_work()): >> 1. a1 gets popped from the entity by the scheduler >> 2. run_job(a1) executes >> 3. a1's scheduled fence gets signaled >> 4. drm_sched_run_job_work() calls drm_sched_run_job_queue() at the end >> 5. The scheduler wakes up and re-selects entities to pop jobs >> 6. Since b1's dependency is cleared, the scheduler can select b1 and >> continue the same flow >> >> >> However, if application A is killed before a1 gets popped by the scheduler, >> then a1, a2, a3 are killed sequentially by drm_sched_entity_kill_jobs_cb(). >> During the kill process, their scheduled fences are still signaled, but the >> kill process itself lacks work_run_job. This means b1's dependency gets >> cleared, but there's no work_run_job to drive the scheduler to continue >> running, or we can saystep 4 in the normal flow is missing, and the >> scheduler enters sleep state, which causes application B to hang. >> So if we add drm_sched_wakeup() in drm_sched_entity_kill_jobs_work() after >> drm_sched_fence_scheduled(), the scheduler can be woken up, and application >> B can continue running after a1's scheduled fence is force signaled. > > Thanks for the detailed explanation. Makes sense. > Maybe you can detail in v3 that this "optimization" consists of the > work item not being scheduled.
Yeah, removing this "optimization" would also be a valid solution to the problem. Christian. > I think that was the piece of the puzzle > I was missing. > > I / DRM tools will also include a link to this thread, so I think that > will then be sufficient. > > Thx > P. > >> >> Thanks, >> Lin >> >> >> >> >> >> From: Philipp Stanner <pha...@mailbox.org> >> Sent: Tuesday, July 15, 2025 17:12 >> To: cao, lin <lin....@amd.com>; Koenig, Christian >> <christian.koe...@amd.com>; pha...@kernel.org <pha...@kernel.org>; >> dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> >> Cc: Yin, ZhenGuo (Chris) <zhenguo....@amd.com>; Deng, Emily >> <emily.d...@amd.com>; d...@kernel.org <d...@kernel.org>; >> matthew.br...@intel.com <matthew.br...@intel.com> >> Subject: Re: [PATCH v2] drm/scheduler: Fix sched hang when killing app with >> dependent jobs >> >> >> On Tue, 2025-07-15 at 03:38 +0000, cao, lin wrote: >>> >>> [AMD Official Use Only - AMD Internal Distribution Only] >>> >>> >>> >>> Hi Christian, Philipp, >>> >>> >>> I modified the commit msg, and I hope it makes more sense: >>> >>> >>> drm/sched: wake up scheduler when killing jobs to prevent hang >> >> nit: >> s/wake/Wake >> >>> >>> >>> When application A submits jobs (a1, a2, a3) and application B submits >>> job b1 with a dependency on a2's scheduler fence, killing application A >>> before run_job(a1) causes drm_sched_entity_kill_jobs_work() to force >>> signal all jobs sequentially. However, the optimization in >>> drm_sched_entity_add_dependency_cb() waits for the fence to be scheduled >>> by adding a callback (drm_sched_entity_clear_dep) instead of immediately >>> waking up the scheduler. When A is killed before its jobs can run, the >>> callback gets triggered but drm_sched_entity_clear_dep() only clears the >>> dependency without waking up the scheduler, causing the scheduler to enter >>> sleep state and application B to hang. >> >> That now reads as if drm_sched_entity_clear_dep() is supposed to wake >> up the scheduler. But you add the wakeup to a different function (the >> work item). >> >> Also the code actually looks like that: >> >> >> xa_erase(&job->dependencies, index); >> if (f && !dma_fence_add_callback(f, &job->finish_cb, >> >> drm_sched_entity_kill_jobs_cb)) >> return; >> >> dma_fence_put(f); >> } >> >> INIT_WORK(&job->work, drm_sched_entity_kill_jobs_work); >> schedule_work(&job->work); >> } >> >> So if adding that callback succeeds we.. return immediately but we.. do >> that for the first dependency, not for all of them? >> >> Oh boy. That code base. We(tm) should look into pimping that up. Also >> lacks documentation everywhere. >> >> >> Anyways. If this solves a bug for you the patch looks fine (i.e., not >> potentially harmful) by me and if the tests succeed we can merge it. >> However, I'd feel better if you could clarify more why that function is >> the right place to solve the bug. >> >> >> Thanks, >> P. >> >> >>> >>> >>> Add drm_sched_wakeup() in entity_kill_job_work() to prevent scheduler >>> sleep and subsequent application hangs. >>> >>> >>> v2: >>> - Move drm_sched_wakeup() to after drm_sched_fence_scheduled() >>> >>> >>> Thanks, >>> Lin >>> >>> >>> From: Koenig, Christian <christian.koe...@amd.com> >>> Sent: Monday, July 14, 2025 21:39 >>> To: pha...@kernel.org <pha...@kernel.org>; cao, lin <lin....@amd.com>; >>> dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> >>> Cc: Yin, ZhenGuo (Chris) <zhenguo....@amd.com>; Deng, Emily >>> <emily.d...@amd.com>; d...@kernel.org <d...@kernel.org>; >>> matthew.br...@intel.com <matthew.br...@intel.com> >>> Subject: Re: [PATCH v2] drm/scheduler: Fix sched hang when killing app with >>> dependent jobs >>> >>> >>> >>> >>> On 14.07.25 15:27, Philipp Stanner wrote: >>>> On Mon, 2025-07-14 at 15:08 +0200, Christian König wrote: >>>>> >>>>> >>>>> On 14.07.25 14:46, Philipp Stanner wrote: >>>>>> regarding the patch subject: the prefix we use for the scheduler >>>>>> is: >>>>>> drm/sched: >>>>>> >>>>>> >>>>>> On Mon, 2025-07-14 at 14:23 +0800, Lin.Cao wrote: >>>>>> >>>>>>> When Application A submits jobs (a1, a2, a3) and application B >>>>>>> submits >>>>>> >>>>>> s/Application/application >>>>>> >>>>>>> job b1 with a dependency on a2's scheduler fence, killing >>>>>>> application A >>>>>>> before run_job(a1) causes drm_sched_entity_kill_jobs_work() to >>>>>>> force >>>>>>> signal all jobs sequentially. However, due to missing >>>>>>> work_run_job or >>>>>>> work_free_job >>>>>>> >>>>>> >>>>>> You probably mean "due to the work items work_run_job and >>>>>> work_free_job >>>>>> not being started […]". >>>>>> >>>>>> However, the call you add, drm_sched_wakeup(), immediately only >>>>>> triggers work_run_job. It's not clear to me why you mention >>>>>> work_free_job, because drm_sched_entity_kill_jobs_work() directly >>>>>> invokes the free_job() callback. >>>>> >>>>> Yeah the description is rather confusing, took me more than one try >>>>> to understand what's going on here as well. Let me try to explain it >>>>> in my words: >>>>> >>>>> When an application A is killed the pending submissions from it are >>>>> not executed, but rather torn down by >>>>> drm_sched_entity_kill_jobs_work(). >>>>> >>>>> If now a submission from application B depends on something >>>>> application A wanted to do on the same scheduler instance the >>>>> function drm_sched_entity_add_dependency_cb() would have optimized >>>>> this dependency and assumed that the scheduler work would already run >>>>> and try to pick a job. >>>>> >>>>> But that isn't the case when the submissions from application A are >>>>> all killed. So make sure to start the scheduler work from >>>>> drm_sched_entity_kill_jobs_work() to handle that case. >>>> >>>> Alright, so the bug then depends on B setting a dependency on A _after_ >>>> A was killed, doesn't it? Because the optimization in >>>> _add_dependency_cb() can only have an effect after A's jobs have been >>>> torn down. >>> >>> No, application A and application B submit right behind each other. >>> Application A is then killed later on. >>> >>> The optimization in _add_dependency_cb() just waits for As submission to be >>> handled by the scheduler, but that never happens because it was killed. >>> >>>> If there is such a timing order problem, the commit message should >>>> mention it. >>>> >>>> The commit message definitely also needs to mention this optimization >>>> in drm_sched_entity_add_dependency_cb() since it's essential for >>>> understanding the bug. >>> >>> +1 >>> >>> Christian. >>> >>>> >>>> >>>> Danke >>>> P. >>>> >>>> >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> >>>>>>> in entity_kill_job_work(), the scheduler enters sleep >>>>>>> state, causing application B hang. >>>>>> >>>>>> So the issue is that if a1 never ran, the scheduler will not >>>>>> continue >>>>>> with the jobs of application B, correct? >>>>>> >>>>>>> >>>>>>> Add drm_sched_wakeup() when entity_kill_job_work() to preventing >>>>>> >>>>>> s/to preventing/to prevent >>>>>> >>>>>>> scheduler sleep and subsequent application hangs. >>>>>>> >>>>>>> v2: >>>>>>> - Move drm_sched_wakeup() to after drm_sched_fence_scheduled() >>>>>> >>>>>> Changelogs are cool and useful, but move them below the Signed-off >>>>>> area >>>>>> with another --- please, like so: >>>>>> >>>>>> Signed-off by: … >>>>>> --- >>>>>> v2: >>>>>> … >>>>>> --- >>>>>> drivers/gpu/drm/scheduler/sched_entity.c | 1 + >>>>>> >>>>>> >>>>>>> >>>>>>> Signed-off-by: Lin.Cao <linca...@amd.com> >>>>>> >>>>>> This fixes a bug. Even a racy bug, it seems. So we need a Fixes tag >>>>>> and >>>>>> put the stable kernel in CC. >>>>>> >>>>>> Please figure out with git blame, git log and git tag --contains >>>>>> which >>>>>> commit your patch fixes and which is the oldest kernel that >>>>>> contains >>>>>> the bug. Then add a Fixes: tag and Cc: the stable kernel. You'll >>>>>> find >>>>>> lots of examples for that in git log. >>>>>> >>>>>> >>>>>> Thx >>>>>> P. >>>>>> >>>>>>> --- >>>>>>> drivers/gpu/drm/scheduler/sched_entity.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c >>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c >>>>>>> index e671aa241720..66f2a43c58fd 100644 >>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c >>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c >>>>>>> @@ -177,6 +177,7 @@ static void >>>>>>> drm_sched_entity_kill_jobs_work(struct work_struct *wrk) >>>>>>> struct drm_sched_job *job = container_of(wrk, >>>>>>> typeof(*job), work); >>>>>>> >>>>>>> drm_sched_fence_scheduled(job->s_fence, NULL); >>>>>>> + drm_sched_wakeup(job->sched); >>>>>>> drm_sched_fence_finished(job->s_fence, -ESRCH); >>>>>>> WARN_ON(job->s_fence->parent); >>>>>>> job->sched->ops->free_job(job); >>>>>> >>>>> >>>> >>> >> >
