Re: [PATCH v2] drm/scheduler: Fix sched hang when killing app with dependent jobs

[Philipp Stanner <phasta@mailbox.org>]

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.

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.


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 <lincao12@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);
> > 
>