[RFC PATCH 0/3] Enforce DRM scheduler reclaim rules

[RFC PATCH 0/3] Enforce DRM scheduler reclaim rules

[Matthew Brost]

Over the past year, I've randomly reviewed new drivers using the DRM
scheduler and repeatedly spotted misuses related to reclaim. This update
to the DRM scheduler aims to catch some of these misuses.

It's quite possible that this change will expose bugs in upstream
drivers, which is why it's being submitted as an RFC."

Matt

Matthew Brost (3):
  workqueue: Add an interface to taint workqueue lockdep with reclaim
  drm/sched: Taint workqueues with reclaim
  drm/sched: Prevent adding dependencies to an armed job

 drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++++-
 include/linux/workqueue.h              | 19 +++++++++++++++++++
 kernel/workqueue.c                     |  9 +++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

-- 
2.34.1

[RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

Drivers often use workqueues that are in the reclaim path (e.g., DRM
scheduler workqueues). It is useful to teach lockdep that memory cannot
be allocated on these workqueues. Add an interface to taint workqueue
lockdep with reclaim.

Cc: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 include/linux/workqueue.h | 19 +++++++++++++++++++
 kernel/workqueue.c        |  9 +++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index dabc351cc127..954c7eb7e225 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
 						1, lockdep_map, ##args))
 #endif
 
+
+#ifdef CONFIG_LOCKDEP
+/**
+ * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
+ * @wq: workqueue to taint with reclaim
+ * gfp: gfp taint
+ *
+ * Drivers often use workqueues that are in the reclaim path (e.g., DRM
+ * scheduler workqueues). It is useful to teach lockdep that memory cannot be
+ * allocated on these workqueues.
+ */
+extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
+#else
+static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
+					   gfp_t gfp)
+{
+}
+#endif
+
 /**
  * alloc_ordered_workqueue - allocate an ordered workqueue
  * @fmt: printf format for the name of the workqueue
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 45320e27a16c..fea410c20b71 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5846,6 +5846,15 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
 	return wq;
 }
 EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
+
+void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp)
+{
+	fs_reclaim_acquire(gfp);
+	lock_map_acquire(wq->lockdep_map);
+	lock_map_release(wq->lockdep_map);
+	fs_reclaim_release(gfp);
+}
+EXPORT_SYMBOL_GPL(taint_reclaim_workqueue);
 #endif
 
 static bool pwq_busy(struct pool_workqueue *pwq)
-- 
2.34.1

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

Hello,

On Tue, Oct 21, 2025 at 02:39:50PM -0700, Matthew Brost wrote:
> Drivers often use workqueues that are in the reclaim path (e.g., DRM
> scheduler workqueues). It is useful to teach lockdep that memory cannot
> be allocated on these workqueues. Add an interface to taint workqueue
> lockdep with reclaim.

Given that it's about reclaim, "memory cannot be allocated" may be a bit
misleading. Can you make the description more accurate? Also, it'd be great
if you can include an example lockdep splat for reference.

> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  include/linux/workqueue.h | 19 +++++++++++++++++++
>  kernel/workqueue.c        |  9 +++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index dabc351cc127..954c7eb7e225 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
>  						1, lockdep_map, ##args))
>  #endif
>  
> +
> +#ifdef CONFIG_LOCKDEP
> +/**
> + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
> + * @wq: workqueue to taint with reclaim
> + * gfp: gfp taint
      ^@

> + *
> + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
> + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
> + * allocated on these workqueues.
> + */
> +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
> +#else
> +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
> +					   gfp_t gfp)

Would a more direct name work better, maybe something like
workqueue_warn_on_reclaim()?

Hmm... would it make sense to tie this to WQ_MEM_RECLAIM - ie. enable it
implicitly on workqueues w/ the flag set?

Thanks.

-- 
tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 21, 2025 at 11:56:30AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 21, 2025 at 02:39:50PM -0700, Matthew Brost wrote:
> > Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > scheduler workqueues). It is useful to teach lockdep that memory cannot
> > be allocated on these workqueues. Add an interface to taint workqueue
> > lockdep with reclaim.
> 
> Given that it's about reclaim, "memory cannot be allocated" may be a bit
> misleading. Can you make the description more accurate? Also, it'd be great
> if you can include an example lockdep splat for reference.
> 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  include/linux/workqueue.h | 19 +++++++++++++++++++
> >  kernel/workqueue.c        |  9 +++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index dabc351cc127..954c7eb7e225 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >  						1, lockdep_map, ##args))
> >  #endif
> >  
> > +
> > +#ifdef CONFIG_LOCKDEP
> > +/**
> > + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
> > + * @wq: workqueue to taint with reclaim
> > + * gfp: gfp taint
>       ^@
> 
> > + *
> > + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
> > + * allocated on these workqueues.
> > + */
> > +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
> > +#else
> > +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
> > +					   gfp_t gfp)
> 
> Would a more direct name work better, maybe something like
> workqueue_warn_on_reclaim()?
> 

Can rename, but perhaps not needed depending on what we land on below.

> Hmm... would it make sense to tie this to WQ_MEM_RECLAIM - ie. enable it
> implicitly on workqueues w/ the flag set?
> 

I had considered this, and for a while I thought WQ_MEM_RECLAIM already
did what I'm suggesting—especially since I’ve spotted bugs in drivers
where I would have expected lockdep to catch them.

In my opinion, this approach is better, but it has a broader kernel-wide
scope and could potentially break some things. My subsequent patches
will likely break one or two DRM drivers, so it might not be a concern
to fix everything that breaks across the kernel. It's up to you which
route we want to take here.

Matt 

> Thanks.
> 
> -- 
> tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 21, 2025 at 03:04:14PM -0700, Matthew Brost wrote:
> On Tue, Oct 21, 2025 at 11:56:30AM -1000, Tejun Heo wrote:
> > Hello,

Missed a comment.

> > 
> > On Tue, Oct 21, 2025 at 02:39:50PM -0700, Matthew Brost wrote:
> > > Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > > scheduler workqueues). It is useful to teach lockdep that memory cannot
> > > be allocated on these workqueues. Add an interface to taint workqueue
> > > lockdep with reclaim.
> > 
> > Given that it's about reclaim, "memory cannot be allocated" may be a bit
> > misleading. Can you make the description more accurate? Also, it'd be great

Can fix the comment. The rule is memory cannot be allocated in the
context of reclaim (e.g., GFP_KERNEL).

> > if you can include an example lockdep splat for reference.

My driver (Xe) doesn't break anything but can hack to trigger a lockdep
warning and include it.

Matt

> > 
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  include/linux/workqueue.h | 19 +++++++++++++++++++
> > >  kernel/workqueue.c        |  9 +++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > > index dabc351cc127..954c7eb7e225 100644
> > > --- a/include/linux/workqueue.h
> > > +++ b/include/linux/workqueue.h
> > > @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> > >  						1, lockdep_map, ##args))
> > >  #endif
> > >  
> > > +
> > > +#ifdef CONFIG_LOCKDEP
> > > +/**
> > > + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
> > > + * @wq: workqueue to taint with reclaim
> > > + * gfp: gfp taint
> >       ^@
> > 
> > > + *
> > > + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > > + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
> > > + * allocated on these workqueues.
> > > + */
> > > +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
> > > +#else
> > > +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
> > > +					   gfp_t gfp)
> > 
> > Would a more direct name work better, maybe something like
> > workqueue_warn_on_reclaim()?
> > 
> 
> Can rename, but perhaps not needed depending on what we land on below.
> 
> > Hmm... would it make sense to tie this to WQ_MEM_RECLAIM - ie. enable it
> > implicitly on workqueues w/ the flag set?
> > 
> 
> I had considered this, and for a while I thought WQ_MEM_RECLAIM already
> did what I'm suggesting—especially since I’ve spotted bugs in drivers
> where I would have expected lockdep to catch them.
> 
> In my opinion, this approach is better, but it has a broader kernel-wide
> scope and could potentially break some things. My subsequent patches
> will likely break one or two DRM drivers, so it might not be a concern
> to fix everything that breaks across the kernel. It's up to you which
> route we want to take here.
> 
> Matt 
> 
> > Thanks.
> > 
> > -- 
> > tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

Hello,

On Tue, Oct 21, 2025 at 03:06:55PM -0700, Matthew Brost wrote:
> > > Given that it's about reclaim, "memory cannot be allocated" may be a bit
> > > misleading. Can you make the description more accurate? Also, it'd be great
> 
> Can fix the comment. The rule is memory cannot be allocated in the
> context of reclaim (e.g., GFP_KERNEL).

Oh, I meant that e.g. GPF_ATOMIC or GFP_NOFS reclaims should be fine. It's
just that we can't recurse into reclaim from WQ_RECLAIM workqueue, right?

Thanks.

-- 
tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 21, 2025 at 01:25:52PM -1000, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 21, 2025 at 03:06:55PM -0700, Matthew Brost wrote:
> > > > Given that it's about reclaim, "memory cannot be allocated" may be a bit
> > > > misleading. Can you make the description more accurate? Also, it'd be great
> > 
> > Can fix the comment. The rule is memory cannot be allocated in the
> > context of reclaim (e.g., GFP_KERNEL).
> 
> Oh, I meant that e.g. GPF_ATOMIC or GFP_NOFS reclaims should be fine. It's
> just that we can't recurse into reclaim from WQ_RECLAIM workqueue, right?
> 

Yes, exactly. Will adjust.

Matt

> Thanks.
> 
> -- 
> tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

Hello,

On Tue, Oct 21, 2025 at 03:04:14PM -0700, Matthew Brost wrote:
> > Hmm... would it make sense to tie this to WQ_MEM_RECLAIM - ie. enable it
> > implicitly on workqueues w/ the flag set?
> 
> I had considered this, and for a while I thought WQ_MEM_RECLAIM already
> did what I'm suggesting—especially since I’ve spotted bugs in drivers
> where I would have expected lockdep to catch them.
> 
> In my opinion, this approach is better, but it has a broader kernel-wide
> scope and could potentially break some things. My subsequent patches
> will likely break one or two DRM drivers, so it might not be a concern
> to fix everything that breaks across the kernel. It's up to you which
> route we want to take here.

Yeah, it is bothersome that WQ_MEM_RECLAIM doesn't currently have a way to
ensure compliance. I just didn't know about the lockdep mechanism. Can you
please update the patch so that WQ_MEM_RECLAIM implicitly enables the
checking?

Thanks.

-- 
tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 21, 2025 at 01:28:31PM -1000, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 21, 2025 at 03:04:14PM -0700, Matthew Brost wrote:
> > > Hmm... would it make sense to tie this to WQ_MEM_RECLAIM - ie. enable it
> > > implicitly on workqueues w/ the flag set?
> > 
> > I had considered this, and for a while I thought WQ_MEM_RECLAIM already
> > did what I'm suggesting—especially since I’ve spotted bugs in drivers
> > where I would have expected lockdep to catch them.
> > 
> > In my opinion, this approach is better, but it has a broader kernel-wide
> > scope and could potentially break some things. My subsequent patches
> > will likely break one or two DRM drivers, so it might not be a concern
> > to fix everything that breaks across the kernel. It's up to you which
> > route we want to take here.
> 
> Yeah, it is bothersome that WQ_MEM_RECLAIM doesn't currently have a way to
> ensure compliance. I just didn't know about the lockdep mechanism. Can you

I agree this is the best route to ensure compliance.

> please update the patch so that WQ_MEM_RECLAIM implicitly enables the
> checking?
> 

Sure, but a bunch of things immediately break—including a convoluted
case in my driver. I can fix the kernel to the extent that my CI catches
issues, and fix any obvious cases through manual inspection. However,
I suspect that if we merge this, we'll be dealing with fallout
throughout a kernel RC cycle.

Matt

> Thanks.
> 
> -- 
> tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

On Tue, Oct 21, 2025 at 06:22:19PM -0700, Matthew Brost wrote:
> > please update the patch so that WQ_MEM_RECLAIM implicitly enables the
> > checking?
> 
> Sure, but a bunch of things immediately break—including a convoluted
> case in my driver. I can fix the kernel to the extent that my CI catches
> issues, and fix any obvious cases through manual inspection. However,
> I suspect that if we merge this, we'll be dealing with fallout
> throughout a kernel RC cycle.

Sure, we're still early in this cycle and can try to resolve as much as
possible and if there's just too much, we can make it optional and so on.

Thanks.

-- 
tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 21, 2025 at 03:51:08PM -1000, Tejun Heo wrote:
> On Tue, Oct 21, 2025 at 06:22:19PM -0700, Matthew Brost wrote:
> > > please update the patch so that WQ_MEM_RECLAIM implicitly enables the
> > > checking?
> > 
> > Sure, but a bunch of things immediately break—including a convoluted
> > case in my driver. I can fix the kernel to the extent that my CI catches
> > issues, and fix any obvious cases through manual inspection. However,
> > I suspect that if we merge this, we'll be dealing with fallout
> > throughout a kernel RC cycle.
> 
> Sure, we're still early in this cycle and can try to resolve as much as
> possible and if there's just too much, we can make it optional and so on.
> 

I’ve come to the conclusion that the entire kernel is going to explode
if all WQ_MEM_RECLAIM workqueues are annotated with lockdep. I made this
change and tried booting Linux, but quickly ran into five issues before
giving up. It seems that many parts of the kernel allocate memory with
GFP_KERNEL or take locks that allocate memory in workqueues marked with
WQ_MEM_RECLAIM.

There are literally hundreds of workqueues created with WQ_MEM_RECLAIM,
both directly [1] and via the create*workqueue helpers, which also set
this flag.

So, what’s your advice here? Personally, I don’t have the bandwidth to
drive fixing the entire kernel. Maybe we could add the annotation
helpers introduced in this series, so drivers that really want to
enforce this rule—like the DRM driver—can opt in? And perhaps we could
add a export Kconfig option that enables this for all WQ_MEM_RECLAIM
workqueues, and let the community gradually enable it and start fixing
their code?

Matt

[1] https://elixir.bootlin.com/linux/v6.17.5/C/ident/WQ_MEM_RECLAIM

> Thanks.
> 
> -- 
> tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Christian König]

On 10/21/25 23:39, Matthew Brost wrote:
> Drivers often use workqueues that are in the reclaim path (e.g., DRM
> scheduler workqueues). It is useful to teach lockdep that memory cannot
> be allocated on these workqueues. Add an interface to taint workqueue
> lockdep with reclaim.

Oh that is so wonderfully evil. I'm absolutely in favor of doing this.

But can't we check for the existing WQ_MEM_RECLAIM flag in the workqueue handling instead?

Additional to that we should also make sure that the same wq is used for timeout and free and that this wq is single threaded *and* has the WQ_MEM_RECLAIM flag set.

Otherwise we run into the same lifetime issue with the job and memory reclaim during device reset as well.

Regards,
Christian.

> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  include/linux/workqueue.h | 19 +++++++++++++++++++
>  kernel/workqueue.c        |  9 +++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index dabc351cc127..954c7eb7e225 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
>  						1, lockdep_map, ##args))
>  #endif
>  
> +
> +#ifdef CONFIG_LOCKDEP
> +/**
> + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
> + * @wq: workqueue to taint with reclaim
> + * gfp: gfp taint
> + *
> + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
> + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
> + * allocated on these workqueues.
> + */
> +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
> +#else
> +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
> +					   gfp_t gfp)
> +{
> +}
> +#endif
> +
>  /**
>   * alloc_ordered_workqueue - allocate an ordered workqueue
>   * @fmt: printf format for the name of the workqueue
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 45320e27a16c..fea410c20b71 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -5846,6 +5846,15 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
>  	return wq;
>  }
>  EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
> +
> +void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp)
> +{
> +	fs_reclaim_acquire(gfp);
> +	lock_map_acquire(wq->lockdep_map);
> +	lock_map_release(wq->lockdep_map);
> +	fs_reclaim_release(gfp);
> +}
> +EXPORT_SYMBOL_GPL(taint_reclaim_workqueue);
>  #endif
>  
>  static bool pwq_busy(struct pool_workqueue *pwq)

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Tue, Oct 28, 2025 at 10:32:54AM +0100, Christian König wrote:
> On 10/21/25 23:39, Matthew Brost wrote:
> > Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > scheduler workqueues). It is useful to teach lockdep that memory cannot
> > be allocated on these workqueues. Add an interface to taint workqueue
> > lockdep with reclaim.
> 
> Oh that is so wonderfully evil. I'm absolutely in favor of doing this.
> 
> But can't we check for the existing WQ_MEM_RECLAIM flag in the workqueue handling instead?
> 

Tejun suggested tying the lockdep annotation to WQ_MEM_RECLAIM, but the
entire kernel explodes because many workqueues throughout Linux don’t
adhere to this rule. Here's a link to my latest reply to Tejun [1].

[1] https://patchwork.freedesktop.org/patch/682494/?series=156284&rev=1#comment_1255380 

> Additional to that we should also make sure that the same wq is used for timeout and free and that this wq is single threaded *and* has the WQ_MEM_RECLAIM flag set.
> 

Currently, free runs on the same work queue as run_job. We could look
into moving it to a separate queue, but that’s a separate issue.

IIRC the workqueue_struct is private and so we can't fish that out in
the DRM scheduler without adding helpers to workqueue layer. Ofc we
could do that too if you think this would be helpful.

> Otherwise we run into the same lifetime issue with the job and memory reclaim during device reset as well.
> 

My patches in this series taint the submit_wq and timeout_wq in the DRM
scheduler [2]. I have a solid understanding of reclaim rules, and this
change helped uncover some convoluted cases in Xe—specifically in our
device reset code involving power management and reclaim [3]. So I can
confirm this has been quite helpful.

Matt

[2] https://patchwork.freedesktop.org/patch/682496/?series=156284&rev=1
[3] https://patchwork.freedesktop.org/series/156292/

> Regards,
> Christian.
> 
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  include/linux/workqueue.h | 19 +++++++++++++++++++
> >  kernel/workqueue.c        |  9 +++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index dabc351cc127..954c7eb7e225 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >  						1, lockdep_map, ##args))
> >  #endif
> >  
> > +
> > +#ifdef CONFIG_LOCKDEP
> > +/**
> > + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
> > + * @wq: workqueue to taint with reclaim
> > + * gfp: gfp taint
> > + *
> > + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
> > + * allocated on these workqueues.
> > + */
> > +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
> > +#else
> > +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
> > +					   gfp_t gfp)
> > +{
> > +}
> > +#endif
> > +
> >  /**
> >   * alloc_ordered_workqueue - allocate an ordered workqueue
> >   * @fmt: printf format for the name of the workqueue
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 45320e27a16c..fea410c20b71 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -5846,6 +5846,15 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> >  	return wq;
> >  }
> >  EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
> > +
> > +void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp)
> > +{
> > +	fs_reclaim_acquire(gfp);
> > +	lock_map_acquire(wq->lockdep_map);
> > +	lock_map_release(wq->lockdep_map);
> > +	fs_reclaim_release(gfp);
> > +}
> > +EXPORT_SYMBOL_GPL(taint_reclaim_workqueue);
> >  #endif
> >  
> >  static bool pwq_busy(struct pool_workqueue *pwq)
> 

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Christian König]

On 10/28/25 21:16, Matthew Brost wrote:
> On Tue, Oct 28, 2025 at 10:32:54AM +0100, Christian König wrote:
>> On 10/21/25 23:39, Matthew Brost wrote:
>>> Drivers often use workqueues that are in the reclaim path (e.g., DRM
>>> scheduler workqueues). It is useful to teach lockdep that memory cannot
>>> be allocated on these workqueues. Add an interface to taint workqueue
>>> lockdep with reclaim.
>>
>> Oh that is so wonderfully evil. I'm absolutely in favor of doing this.
>>
>> But can't we check for the existing WQ_MEM_RECLAIM flag in the workqueue handling instead?
>>
> 
> Tejun suggested tying the lockdep annotation to WQ_MEM_RECLAIM, but the
> entire kernel explodes because many workqueues throughout Linux don’t
> adhere to this rule. Here's a link to my latest reply to Tejun [1].
> 
> [1] https://patchwork.freedesktop.org/patch/682494/?series=156284&rev=1#comment_1255380 

Sorry my fault, I hadn't read up to the latest discussion when I wrote the mail.

My educated guess is that a lot of wq just set WQ_MEM_RECLAIM to be guaranteed to to start even under memory pressure.

So yeah probably best to keep your approach here for now and somebody from core MM should take a look at cleaning it up later on.
>> Additional to that we should also make sure that the same wq is used for timeout and free and that this wq is single threaded *and* has the WQ_MEM_RECLAIM flag set.
>>
> 
> Currently, free runs on the same work queue as run_job. We could look
> into moving it to a separate queue, but that’s a separate issue.

We really need to make sure the free and timeout wq are the same and single threaded.

The hack the scheduler currently does with removing and re-inserting the job on a timeout is something we should really try to fix.

> IIRC the workqueue_struct is private and so we can't fish that out in
> the DRM scheduler without adding helpers to workqueue layer. Ofc we
> could do that too if you think this would be helpful.

I might be wrong, but IIRC there was a helper to get the flags from the wq.

That should be enough to test if it is single threaded or not.

> 
>> Otherwise we run into the same lifetime issue with the job and memory reclaim during device reset as well.
>>
> 
> My patches in this series taint the submit_wq and timeout_wq in the DRM
> scheduler [2]. I have a solid understanding of reclaim rules, and this
> change helped uncover some convoluted cases in Xe—specifically in our
> device reset code involving power management and reclaim [3]. So I can
> confirm this has been quite helpful.

Yeah, completely agree. We most likely have quite a bunch of issues in our reset code path as well.

Regards,
Christian.

> 
> Matt
> 
> [2] https://patchwork.freedesktop.org/patch/682496/?series=156284&rev=1
> [3] https://patchwork.freedesktop.org/series/156292/
> 
>> Regards,
>> Christian.
>>
>>>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>  include/linux/workqueue.h | 19 +++++++++++++++++++
>>>  kernel/workqueue.c        |  9 +++++++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
>>> index dabc351cc127..954c7eb7e225 100644
>>> --- a/include/linux/workqueue.h
>>> +++ b/include/linux/workqueue.h
>>> @@ -553,6 +553,25 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
>>>  						1, lockdep_map, ##args))
>>>  #endif
>>>  
>>> +
>>> +#ifdef CONFIG_LOCKDEP
>>> +/**
>>> + * taint_reclaim_workqueue - taint workqueue lockdep map with reclaim
>>> + * @wq: workqueue to taint with reclaim
>>> + * gfp: gfp taint
>>> + *
>>> + * Drivers often use workqueues that are in the reclaim path (e.g., DRM
>>> + * scheduler workqueues). It is useful to teach lockdep that memory cannot be
>>> + * allocated on these workqueues.
>>> + */
>>> +extern void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp);
>>> +#else
>>> +static inline void taint_reclaim_workqueue(struct workqueue_struct *wq,
>>> +					   gfp_t gfp)
>>> +{
>>> +}
>>> +#endif
>>> +
>>>  /**
>>>   * alloc_ordered_workqueue - allocate an ordered workqueue
>>>   * @fmt: printf format for the name of the workqueue
>>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>>> index 45320e27a16c..fea410c20b71 100644
>>> --- a/kernel/workqueue.c
>>> +++ b/kernel/workqueue.c
>>> @@ -5846,6 +5846,15 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
>>>  	return wq;
>>>  }
>>>  EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
>>> +
>>> +void taint_reclaim_workqueue(struct workqueue_struct *wq, gfp_t gfp)
>>> +{
>>> +	fs_reclaim_acquire(gfp);
>>> +	lock_map_acquire(wq->lockdep_map);
>>> +	lock_map_release(wq->lockdep_map);
>>> +	fs_reclaim_release(gfp);
>>> +}
>>> +EXPORT_SYMBOL_GPL(taint_reclaim_workqueue);
>>>  #endif
>>>  
>>>  static bool pwq_busy(struct pool_workqueue *pwq)
>>

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

Hello,

On Tue, Oct 28, 2025 at 01:16:43PM -0700, Matthew Brost wrote:
> On Tue, Oct 28, 2025 at 10:32:54AM +0100, Christian König wrote:
> > On 10/21/25 23:39, Matthew Brost wrote:
> > > Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > > scheduler workqueues). It is useful to teach lockdep that memory cannot
> > > be allocated on these workqueues. Add an interface to taint workqueue
> > > lockdep with reclaim.
> > 
> > Oh that is so wonderfully evil. I'm absolutely in favor of doing this.
> > 
> > But can't we check for the existing WQ_MEM_RECLAIM flag in the workqueue handling instead?
> > 
> 
> Tejun suggested tying the lockdep annotation to WQ_MEM_RECLAIM, but the
> entire kernel explodes because many workqueues throughout Linux don’t
> adhere to this rule. Here's a link to my latest reply to Tejun [1].

How about making it a WQ flag?

Thanks.

-- 
tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Matthew Brost]

On Wed, Oct 29, 2025 at 05:06:46AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 28, 2025 at 01:16:43PM -0700, Matthew Brost wrote:
> > On Tue, Oct 28, 2025 at 10:32:54AM +0100, Christian König wrote:
> > > On 10/21/25 23:39, Matthew Brost wrote:
> > > > Drivers often use workqueues that are in the reclaim path (e.g., DRM
> > > > scheduler workqueues). It is useful to teach lockdep that memory cannot
> > > > be allocated on these workqueues. Add an interface to taint workqueue
> > > > lockdep with reclaim.
> > > 
> > > Oh that is so wonderfully evil. I'm absolutely in favor of doing this.
> > > 
> > > But can't we check for the existing WQ_MEM_RECLAIM flag in the workqueue handling instead?
> > > 
> > 
> > Tejun suggested tying the lockdep annotation to WQ_MEM_RECLAIM, but the
> > entire kernel explodes because many workqueues throughout Linux don’t
> > adhere to this rule. Here's a link to my latest reply to Tejun [1].
> 
> How about making it a WQ flag?
> 

That could work too. We want to enforce rules of drivers actually set
these flags setting passing workqueues to the DRM scheduler. Any
objection to adding helpers to the workqueue layer to fish the
information we'd like to enforce?

Matt

> Thanks.
> 
> -- 
> tejun

Re: [RFC PATCH 1/3] workqueue: Add an interface to taint workqueue lockdep with reclaim

[Tejun Heo]

On Wed, Oct 29, 2025 at 09:46:15AM -0700, Matthew Brost wrote:
> On Wed, Oct 29, 2025 at 05:06:46AM -1000, Tejun Heo wrote:
> > How about making it a WQ flag?
> 
> That could work too. We want to enforce rules of drivers actually set
> these flags setting passing workqueues to the DRM scheduler. Any
> objection to adding helpers to the workqueue layer to fish the
> information we'd like to enforce?

Difficult to tell definitively without looking at the helpers but no
objections to the general idea.

Thanks.

-- 
tejun

[RFC PATCH 2/3] drm/sched: Taint workqueues with reclaim

[Matthew Brost]

Multiple drivers seemingly do not understand the role of DMA fences in
the reclaim path. As a result, DRM scheduler workqueues, which are part
of the fence signaling path, must not allocate memory. This patch
teaches lockdep to recognize these rules in order to catch driver-side
bugs.

Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index c39f0245e3a9..676484dd3ea3 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1368,6 +1368,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 	atomic64_set(&sched->job_id_count, 0);
 	sched->pause_submit = false;
 
+	taint_reclaim_workqueue(sched->submit_wq, GFP_KERNEL);
+	taint_reclaim_workqueue(sched->timeout_wq, GFP_KERNEL);
+
 	sched->ready = true;
 	return 0;
 Out_unroll:
-- 
2.34.1

Re: [RFC PATCH 2/3] drm/sched: Taint workqueues with reclaim

[Philipp Stanner]

On Tue, 2025-10-21 at 14:39 -0700, Matthew Brost wrote:
> Multiple drivers seemingly do not understand the role of DMA fences in
> the reclaim path. As a result, 
> 

result of what? The "role of DMA fences"?

> DRM scheduler workqueues, which are part
> of the fence signaling path, must not allocate memory.
> 

Should be phrased differently. The actual rule here is "The GPU
scheduler's workqueues can be used for memory reclaim. Because of that,
work items on these queues must not allocate memory."

--

In general, I often read in commits or discussions about this or that
"rule", especially "DMA fence rules", but they're often not detailed
very much.


P.

>  This patch
> teaches lockdep to recognize these rules in order to catch driver-side
> bugs.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index c39f0245e3a9..676484dd3ea3 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1368,6 +1368,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
>  	atomic64_set(&sched->job_id_count, 0);
>  	sched->pause_submit = false;
>  
> +	taint_reclaim_workqueue(sched->submit_wq, GFP_KERNEL);
> +	taint_reclaim_workqueue(sched->timeout_wq, GFP_KERNEL);
> +
>  	sched->ready = true;
>  	return 0;
>  Out_unroll:

Re: [RFC PATCH 2/3] drm/sched: Taint workqueues with reclaim

[Matthew Brost]

On Mon, Oct 27, 2025 at 12:03:33PM +0100, Philipp Stanner wrote:
> On Tue, 2025-10-21 at 14:39 -0700, Matthew Brost wrote:
> > Multiple drivers seemingly do not understand the role of DMA fences in
> > the reclaim path. As a result, 
> > 
> 
> result of what? The "role of DMA fences"?
> 
> > DRM scheduler workqueues, which are part
> > of the fence signaling path, must not allocate memory.
> > 
> 
> Should be phrased differently. The actual rule here is "The GPU
> scheduler's workqueues can be used for memory reclaim. Because of that,
> work items on these queues must not allocate memory."
> 

Sure, will reword.

> --
> 
> In general, I often read in commits or discussions about this or that
> "rule", especially "DMA fence rules", but they're often not detailed
> very much.
>

Yes, I kinda assume the audience reviewing any dma-buf or drm-sched
really understand the "DMA fence rules" compare to driver devs which
often do not really get this concept. Taining the work queues here will
help driver devs avoid mistakes and hopefully along the way get them to
point where they understand "DMA fence rules" - it took me a few years
to really get this rules.

Matt
 
> 
> P.
> 
> >  This patch
> > teaches lockdep to recognize these rules in order to catch driver-side
> > bugs.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Philipp Stanner <phasta@kernel.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/scheduler/sched_main.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index c39f0245e3a9..676484dd3ea3 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -1368,6 +1368,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
> >  	atomic64_set(&sched->job_id_count, 0);
> >  	sched->pause_submit = false;
> >  
> > +	taint_reclaim_workqueue(sched->submit_wq, GFP_KERNEL);
> > +	taint_reclaim_workqueue(sched->timeout_wq, GFP_KERNEL);
> > +
> >  	sched->ready = true;
> >  	return 0;
> >  Out_unroll:
> 

[RFC PATCH 3/3] drm/sched: Prevent adding dependencies to an armed job

[Matthew Brost]

According to the DMA scheduler documentation, once a job is armed, it
must be pushed. Drivers should avoid calling the failing code path that
attempts to add dependencies after a job has been armed. This change
enforces that rule.

Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 676484dd3ea3..436cb2844161 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -873,7 +873,8 @@ EXPORT_SYMBOL(drm_sched_job_arm);
  * @job: scheduler job to add the dependencies to
  * @fence: the dma_fence to add to the list of dependencies.
  *
- * Note that @fence is consumed in both the success and error cases.
+ * Note that @fence is consumed in both the success and error cases. This
+ * function cannot be called if the job is armed.
  *
  * Returns:
  * 0 on success, or an error on failing to expand the array.
@@ -886,6 +887,10 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
 	u32 id = 0;
 	int ret;
 
+	/* Do not allow additional dependencies when job is armed */
+	if (WARN_ON_ONCE(job->sched))
+		return -EINVAL;
+
 	if (!fence)
 		return 0;
 
-- 
2.34.1

Re: [RFC PATCH 3/3] drm/sched: Prevent adding dependencies to an armed job

[Philipp Stanner]

I've got a kernel.org addr by now by the way

On Tue, 2025-10-21 at 14:39 -0700, Matthew Brost wrote:
> According to the DMA scheduler documentation, once a job is armed, it
> must be pushed. Drivers should avoid calling the failing code path that
> attempts to add dependencies after a job has been armed.
> 

Why is that a "failing code path"?

The issue with adding callbacks is that adding them to an already
signaled fence is a bad idea. I'm not sure if it's illegal, though.
dma_fence_add_cb() merely returns an error then, but the driver could
in priniciple then execute its cb code itself.

And even if we agree that this is a hard rule that must be followed,
then drm_sched_job_arm() *might* not be the right place, because just
because a job is armed doesn't mean that its fence is about to get
signaled. drm_sched_entity_push_job() would be the critical place.


>  This change
> enforces that rule.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 676484dd3ea3..436cb2844161 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -873,7 +873,8 @@ EXPORT_SYMBOL(drm_sched_job_arm);
>   * @job: scheduler job to add the dependencies to
>   * @fence: the dma_fence to add to the list of dependencies.
>   *
> - * Note that @fence is consumed in both the success and error cases.
> + * Note that @fence is consumed in both the success and error cases. This
> + * function cannot be called if the job is armed.
>   *
>   * Returns:
>   * 0 on success, or an error on failing to expand the array.
> @@ -886,6 +887,10 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
>  	u32 id = 0;
>  	int ret;
>  
> +	/* Do not allow additional dependencies when job is armed */
> +	if (WARN_ON_ONCE(job->sched))

One would probably want an 'armed' boolean for that. At the very least
one wants to document in the struct's docstring that job->sched has
this semantic meaning. Otherwise it's only obvious for people who have
been hacking on the scheduler for years.


By the way I think that we use WARN_ON*() too much in DRM. It generates
difficult to read, non-descriptive error messages compared to
dev_warn() and similar helpers, and it's often a bit overkill. I would
only use it when there is no other choice, such as in an interrupt-
handler or widely used void func() where you cannot simply add a return
code.


P.

> +		return -EINVAL;


> +
>  	if (!fence)
>  		return 0;
>  

Re: [RFC PATCH 3/3] drm/sched: Prevent adding dependencies to an armed job

[Matthew Brost]

On Mon, Oct 27, 2025 at 12:13:58PM +0100, Philipp Stanner wrote:
> I've got a kernel.org addr by now by the way
> 
> On Tue, 2025-10-21 at 14:39 -0700, Matthew Brost wrote:
> > According to the DMA scheduler documentation, once a job is armed, it
> > must be pushed. Drivers should avoid calling the failing code path that
> > attempts to add dependencies after a job has been armed.
> > 
> 
> Why is that a "failing code path"?
> 

I noticed this after I sent - it should something like:

'avoid calling a possible failing code path, which allocates memory.'

I can make this a bit more clear.

> The issue with adding callbacks is that adding them to an already
> signaled fence is a bad idea. I'm not sure if it's illegal, though.
> dma_fence_add_cb() merely returns an error then, but the driver could
> in priniciple then execute its cb code itself.
> 
> And even if we agree that this is a hard rule that must be followed,
> then drm_sched_job_arm() *might* not be the right place, because just
> because a job is armed doesn't mean that its fence is about to get
> signaled. drm_sched_entity_push_job() would be the critical place.
>

I think this break our rule once arm is called, push must be called as
adding dependencies can possibly fail. This rule is called out in your
documentation patch too. I've seen 2 driver posted in the past year add
dependencies after arming, so I figured lets catch this misuse in the
scheduler.

Matt

> 
> >  This change
> > enforces that rule.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Philipp Stanner <phasta@kernel.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/scheduler/sched_main.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 676484dd3ea3..436cb2844161 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -873,7 +873,8 @@ EXPORT_SYMBOL(drm_sched_job_arm);
> >   * @job: scheduler job to add the dependencies to
> >   * @fence: the dma_fence to add to the list of dependencies.
> >   *
> > - * Note that @fence is consumed in both the success and error cases.
> > + * Note that @fence is consumed in both the success and error cases. This
> > + * function cannot be called if the job is armed.
> >   *
> >   * Returns:
> >   * 0 on success, or an error on failing to expand the array.
> > @@ -886,6 +887,10 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
> >  	u32 id = 0;
> >  	int ret;
> >  
> > +	/* Do not allow additional dependencies when job is armed */
> > +	if (WARN_ON_ONCE(job->sched))
> 
> One would probably want an 'armed' boolean for that. At the very least
> one wants to document in the struct's docstring that job->sched has
> this semantic meaning. Otherwise it's only obvious for people who have
> been hacking on the scheduler for years.
> 
> 
> By the way I think that we use WARN_ON*() too much in DRM. It generates
> difficult to read, non-descriptive error messages compared to
> dev_warn() and similar helpers, and it's often a bit overkill. I would
> only use it when there is no other choice, such as in an interrupt-
> handler or widely used void func() where you cannot simply add a return
> code.
> 
> 
> P.
> 
> > +		return -EINVAL;
> 
> 
> > +
> >  	if (!fence)
> >  		return 0;
> >  
> 

Re: [RFC PATCH 3/3] drm/sched: Prevent adding dependencies to an armed job

[Philipp Stanner]

On Mon, 2025-10-27 at 09:56 -0700, Matthew Brost wrote:
> On Mon, Oct 27, 2025 at 12:13:58PM +0100, Philipp Stanner wrote:
> > I've got a kernel.org addr by now by the way
> > 
> > On Tue, 2025-10-21 at 14:39 -0700, Matthew Brost wrote:
> > > According to the DMA scheduler documentation, once a job is armed, it
> > > must be pushed. Drivers should avoid calling the failing code path that
> > > attempts to add dependencies after a job has been armed.
> > > 
> > 
> > Why is that a "failing code path"?
> > 
> 
> I noticed this after I sent - it should something like:
> 
> 'avoid calling a possible failing code path, which allocates memory.'
> 
> I can make this a bit more clear.
> 
> > The issue with adding callbacks is that adding them to an already
> > signaled fence is a bad idea. I'm not sure if it's illegal, though.
> > dma_fence_add_cb() merely returns an error then, but the driver could
> > in priniciple then execute its cb code itself.
> > 
> > And even if we agree that this is a hard rule that must be followed,
> > then drm_sched_job_arm() *might* not be the right place, because just
> > because a job is armed doesn't mean that its fence is about to get
> > signaled. drm_sched_entity_push_job() would be the critical place.
> > 
> 
> I think this break our rule once arm is called, push must be called as
> adding dependencies can possibly fail. This rule is called out in your
> documentation patch too. I've seen 2 driver posted in the past year add
> dependencies after arming, so I figured lets catch this misuse in the
> scheduler.

We can establish that as a rule, I'm OK with that.

P.