Re: [PATCH 1/4] drm/sched: Mark scheduler work queues with WQ_MEM_RECLAIM

[Philipp Stanner <pstanner@redhat.com>]

On Mon, 2024-10-21 at 10:57 -0700, Matthew Brost wrote:
> DRM scheduler work queues are used to submit jobs, jobs are in the
> path

"scheduler work queues" is very generic, how about
"drm_gpu_scheduler.submit_wq is used to submit jobs, [...]"

> or dma-fences, and dma-fences are in the path of reclaim. Mark

s/or/of

> scheduler
> work queues with WQ_MEM_RECLAIM so these work queues can continue to
> make forward progress during reclaim.

It is just *one* queue (per scheduler) really, isn't it?

If the change above is applied, could just say: "Create the work queue
with WQ_MEM_RECLAIM so it can continue [...]"

So for my understanding: is this a performance optimization or is it a
bug? IOW, would forward progress just be delayed or entirely prevented?
Would be cool to state that a bit more clearly in the commit message.

Work-queue docu says "MUST":

``WQ_MEM_RECLAIM`` All wq which might be used in the memory reclaim
paths **MUST** have this flag set. The wq is guaranteed to have at
least one execution context regardless of memory pressure.

So it seems to me that this fixes a bug? Should it be backported in
your opinion?


> 
> Cc: Luben Tuikov <ltuikov89@gmail.com>

btw., how did you send this email? I couldn't find Luben on CC. Added
him.

Thx,
P.

> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 6e4d004d09ce..567811957c0f 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1275,10 +1275,10 @@ int drm_sched_init(struct drm_gpu_scheduler
> *sched,
>   sched->own_submit_wq = false;
>   } else {
>  #ifdef CONFIG_LOCKDEP
> - sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name, 0,
> + sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name,
> WQ_MEM_RECLAIM,
>          &drm_sched_lockdep_map);
>  #else
> - sched->submit_wq = alloc_ordered_workqueue(name, 0);
> + sched->submit_wq = alloc_ordered_workqueue(name, WQ_MEM_RECLAIM);
>  #endif
>   if (!sched->submit_wq)
>   return -ENOMEM;