Re: drm/sched: run queues freed before the TDR that drm_sched_fini() waits for

[Christian König]

On 9/10/26 10:44, Donggeun Yoo wrote:
> On 9/10/26 09:32, Christian König wrote:
>> Amdgpu shouldn't do that any more.
> 
> Correct, and I should have checked before writing it - 182bdd59be41
> ("drm/amdgpu: deprecate guilty handling") removed it. The callers left are
> etnaviv, lima, panfrost and v3d. v3d is the one I should have named.
> 
>> That was an extremely ugly hack applied long long time ago because amdgpu
>> was broken at that time and didn't waited for
>> drm_sched_entity_flush()/drm_sched_entity_fini() before calling
>> drm_sched_fini().
> 
> Understood, I am dropping that half of the argument.
> 
>> No it doesn't. You quoted the wrong code, this is what really matters:
>>
>>         drm_sched_wqueue_stop(sched);
>>
>>         for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
>>                 kfree(sched->sched_rq[i]);
> 
> I am not sure I follow this one.

What I wanted to note is that the line you pointed out is irrelevant, it was only added as a (very hacky) workaround for amdgpu and should most likely be entirely removed from drm_sched_fini.

> If the point is that
> drm_sched_wqueue_stop() has already quiesced the users of the run queues
> by the time the loop runs, I cannot find where it covers the timeout work:

Before calling drm_sched_fini the driver must ensure that all HW fences are signaled. Those HW fences then signal the scheduler fence and terminate the timeout handling.

But it is correct that drm_sched_fini() needs to make sure that this terminating the timeout handling has propagated throughout the system.

>         WRITE_ONCE(sched->pause_submit, true);
>         cancel_work_sync(&sched->work_run_job);
>         cancel_work_sync(&sched->work_free_job);
> 
> work_tdr is queued on sched->timeout_wq and is only canceled by the
> cancel_delayed_work_sync() below the loop, so a timeout handler can still
> be running while the run queues are freed. Is there something else that
> rules that out? And if I have misread your point, please elaborate.

That is a really good point, canceling the timeout handler should indeed happen before freeing the runqueues.

> On how I got there: the KUnit case never signals the hardware fence, which
> is what keeps the handler inside timedout_job() while drm_sched_fini() runs.

Oh, that is completely broken behavior of the KUnit test case. I suggest to fix that as well.

> That breaks the rule that all run_job() fences are signaled before
> drm_sched_fini(), so a correct driver should not reach this, and I have no
> reproducer that does not cheat that way. The same caveat is in the patch.
> 
> I am writing up the patch Philipp asked for. The only change is moving the
> kfree loop down beside kfree(sched->sched_rq); no new code.

That sounds reasonable to me as well, yes.

Regards,
Christian.

> 
> Regards,
> Donggeun