[PATCH v3 0/5] Use user-defined workqueue lockdep map for drm sched

[PATCH v3 0/5] Use user-defined workqueue lockdep map for drm sched

[Matthew Brost]

By default, each DRM scheduler instance creates an ordered workqueue for
submission, and each workqueue creation allocates a new lockdep map.
This becomes problematic when a DRM scheduler is created for every user
queue (e.g., in DRM drivers with firmware schedulers like Xe) due to the
limited number of available lockdep maps. With numerous user queues
being created and destroyed, lockdep may run out of maps, leading to
lockdep being disabled. Xe mitigated this by creating a pool of
workqueues for DRM scheduler use. However, this approach also encounters
issues if the driver is unloaded and reloaded multiple times or if many
VFs are probed.

To address this, we propose creating a single lockdep map for all DRM
scheduler workqueues, which will also resolve issues for other DRM
drivers that create a DRM scheduler per user queue.

This solution has been tested by unloading and reloading the Xe driver.
Before this series, around 30 driver reloads would result in lockdep
being turned off. After implementing the series, the driver can be
unloaded and reloaded hundreds of times without issues.

v2:
 - Split workqueue changes into multiple patches
 - Add alloc_workqueue_lockdep_map (Tejun)
 - Drop RFC
v3:
 - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
 - static inline alloc_ordered_workqueue_lockdep_map (Tejun)

Matt

Matthew Brost (5):
  workqueue: Split alloc_workqueue into internal function and lockdep
    init
  workqueue: Change workqueue lockdep map to pointer
  workqueue: Add interface for user-defined workqueue lockdep map
  drm/sched: Use drm sched lockdep map for submit_wq
  drm/xe: Drop GuC submit_wq pool

 drivers/gpu/drm/scheduler/sched_main.c | 11 ++++
 drivers/gpu/drm/xe/xe_guc_submit.c     | 60 +--------------------
 drivers/gpu/drm/xe/xe_guc_types.h      |  7 ---
 include/linux/workqueue.h              | 52 ++++++++++++++++++
 kernel/workqueue.c                     | 75 ++++++++++++++++++++------
 5 files changed, 124 insertions(+), 81 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] workqueue: Split alloc_workqueue into internal function and lockdep init

[Matthew Brost]

Will help enable user-defined lockdep maps for workqueues.

Cc: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 kernel/workqueue.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1745ca788ede..90a98c9b0ac6 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5612,9 +5612,9 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
 }
 
 __printf(1, 4)
-struct workqueue_struct *alloc_workqueue(const char *fmt,
-					 unsigned int flags,
-					 int max_active, ...)
+static struct workqueue_struct *__alloc_workqueue(const char *fmt,
+						  unsigned int flags,
+						  int max_active, ...)
 {
 	va_list args;
 	struct workqueue_struct *wq;
@@ -5680,12 +5680,11 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	INIT_LIST_HEAD(&wq->flusher_overflow);
 	INIT_LIST_HEAD(&wq->maydays);
 
-	wq_init_lockdep(wq);
 	INIT_LIST_HEAD(&wq->list);
 
 	if (flags & WQ_UNBOUND) {
 		if (alloc_node_nr_active(wq->node_nr_active) < 0)
-			goto err_unreg_lockdep;
+			goto err_free_wq;
 	}
 
 	/*
@@ -5724,9 +5723,6 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 		kthread_flush_worker(pwq_release_worker);
 		free_node_nr_active(wq->node_nr_active);
 	}
-err_unreg_lockdep:
-	wq_unregister_lockdep(wq);
-	wq_free_lockdep(wq);
 err_free_wq:
 	free_workqueue_attrs(wq->unbound_attrs);
 	kfree(wq);
@@ -5737,6 +5733,25 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	destroy_workqueue(wq);
 	return NULL;
 }
+
+__printf(1, 4)
+struct workqueue_struct *alloc_workqueue(const char *fmt,
+					 unsigned int flags,
+					 int max_active, ...)
+{
+	struct workqueue_struct *wq;
+	va_list args;
+
+	va_start(args, max_active);
+	wq = __alloc_workqueue(fmt, flags, max_active, args);
+	va_end(args);
+	if (!wq)
+		return NULL;
+
+	wq_init_lockdep(wq);
+
+	return wq;
+}
 EXPORT_SYMBOL_GPL(alloc_workqueue);
 
 static bool pwq_busy(struct pool_workqueue *pwq)
-- 
2.34.1

Re: [PATCH v3 1/5] workqueue: Split alloc_workqueue into internal function and lockdep init

[kernel test robot]

Hello,

kernel test robot noticed "sysfs:cannot_create_duplicate_filename" on:

commit: 589686d5b8d589e30478cfb8db2e8e2cd54c20e9 ("[PATCH v3 1/5] workqueue: Split alloc_workqueue into internal function and lockdep init")
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Brost/workqueue-Split-alloc_workqueue-into-internal-function-and-lockdep-init/20240810-122131
base: https://git.kernel.org/cgit/linux/kernel/git/tj/wq.git for-next
patch link: https://lore.kernel.org/all/20240809222827.3211998-2-matthew.brost@intel.com/
patch subject: [PATCH v3 1/5] workqueue: Split alloc_workqueue into internal function and lockdep init

in testcase: boot

compiler: clang-18
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+----------------------------------------+------------+------------+
|                                        | 3b47e19ebc | 589686d5b8 |
+----------------------------------------+------------+------------+
| sysfs:cannot_create_duplicate_filename | 0          | 18         |
+----------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202408121610.d1cdf757-oliver.sang@intel.com


[  OK  ] Started User Login Management.
Starting LSB: Load kernel image with kexec...
[   11.888312][  T131] ata_piix 0000:00:01.1: version 2.13
[  OK  ] Started OpenBSD Secure Shell server.
[   11.890919][  T131] scsi host0: ata_piix
[   11.893885][  T131] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scsi_tmf_4945632'
[   11.895088][  T131] CPU: 0 UID: 0 PID: 131 Comm: systemd-udevd Not tainted 6.11.0-rc1-00010-g589686d5b8d5 #1
[   11.896222][  T131] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[   11.897360][  T131] Call Trace:
[   11.897806][  T131]  <TASK>
[ 11.898206][ T131] dump_stack_lvl (kbuild/src/consumer/lib/dump_stack.c:121) 
[ 11.898801][ T131] sysfs_create_dir_ns (kbuild/src/consumer/fs/sysfs/dir.c:32 kbuild/src/consumer/fs/sysfs/dir.c:63) 
[ 11.899453][ T131] kobject_add_internal (kbuild/src/consumer/lib/kobject.c:74 kbuild/src/consumer/lib/kobject.c:240) 
[ 11.900074][ T131] kobject_add (kbuild/src/consumer/lib/kobject.c:430) 


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240812/202408121610.d1cdf757-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 2/5] workqueue: Change workqueue lockdep map to pointer

[Matthew Brost]

Will help enable user-defined lockdep maps for workqueues.

Cc: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 kernel/workqueue.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 90a98c9b0ac6..24df85589dc0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -364,7 +364,8 @@ struct workqueue_struct {
 #ifdef CONFIG_LOCKDEP
 	char			*lock_name;
 	struct lock_class_key	key;
-	struct lockdep_map	lockdep_map;
+	struct lockdep_map	__lockdep_map;
+	struct lockdep_map	*lockdep_map;
 #endif
 	char			name[WQ_NAME_LEN]; /* I: workqueue name */
 
@@ -3203,7 +3204,7 @@ __acquires(&pool->lock)
 	lockdep_start_depth = lockdep_depth(current);
 	/* see drain_dead_softirq_workfn() */
 	if (!bh_draining)
-		lock_map_acquire(&pwq->wq->lockdep_map);
+		lock_map_acquire(pwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	/*
 	 * Strictly speaking we should mark the invariant state without holding
@@ -3237,7 +3238,7 @@ __acquires(&pool->lock)
 	pwq->stats[PWQ_STAT_COMPLETED]++;
 	lock_map_release(&lockdep_map);
 	if (!bh_draining)
-		lock_map_release(&pwq->wq->lockdep_map);
+		lock_map_release(pwq->wq->lockdep_map);
 
 	if (unlikely((worker->task && in_atomic()) ||
 		     lockdep_depth(current) != lockdep_start_depth ||
@@ -3873,8 +3874,8 @@ static void touch_wq_lockdep_map(struct workqueue_struct *wq)
 	if (wq->flags & WQ_BH)
 		local_bh_disable();
 
-	lock_map_acquire(&wq->lockdep_map);
-	lock_map_release(&wq->lockdep_map);
+	lock_map_acquire(wq->lockdep_map);
+	lock_map_release(wq->lockdep_map);
 
 	if (wq->flags & WQ_BH)
 		local_bh_enable();
@@ -3908,7 +3909,7 @@ void __flush_workqueue(struct workqueue_struct *wq)
 	struct wq_flusher this_flusher = {
 		.list = LIST_HEAD_INIT(this_flusher.list),
 		.flush_color = -1,
-		.done = COMPLETION_INITIALIZER_ONSTACK_MAP(this_flusher.done, wq->lockdep_map),
+		.done = COMPLETION_INITIALIZER_ONSTACK_MAP(this_flusher.done, (*wq->lockdep_map)),
 	};
 	int next_color;
 
@@ -4768,7 +4769,8 @@ static void wq_init_lockdep(struct workqueue_struct *wq)
 		lock_name = wq->name;
 
 	wq->lock_name = lock_name;
-	lockdep_init_map(&wq->lockdep_map, lock_name, &wq->key, 0);
+	wq->lockdep_map = &wq->__lockdep_map;
+	lockdep_init_map(wq->lockdep_map, lock_name, &wq->key, 0);
 }
 
 static void wq_unregister_lockdep(struct workqueue_struct *wq)
-- 
2.34.1

[PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Matthew Brost]

Add an interface for a user-defined workqueue lockdep map, which is
helpful when multiple workqueues are created for the same purpose. This
also helps avoid leaking lockdep maps on each workqueue creation.

v2:
 - Add alloc_workqueue_lockdep_map (Tejun)
v3:
 - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
 - static inline alloc_ordered_workqueue_lockdep_map (Tejun)

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 | 52 +++++++++++++++++++++++++++++++++++++++
 kernel/workqueue.c        | 28 +++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 4eb8f9563136..8ccbf510880b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -507,6 +507,58 @@ void workqueue_softirq_dead(unsigned int cpu);
 __printf(1, 4) struct workqueue_struct *
 alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
 
+#ifdef CONFIG_LOCKDEP
+/**
+ * alloc_workqueue_lockdep_map - allocate a workqueue with user-defined lockdep_map
+ * @fmt: printf format for the name of the workqueue
+ * @flags: WQ_* flags
+ * @max_active: max in-flight work items, 0 for default
+ * @lockdep_map: user-defined lockdep_map
+ * @...: args for @fmt
+ *
+ * Same as alloc_workqueue but with the a user-define lockdep_map. Useful for
+ * workqueues created with the same purpose and to avoid leaking a lockdep_map
+ * on each workqueue creation.
+ *
+ * RETURNS:
+ * Pointer to the allocated workqueue on success, %NULL on failure.
+ */
+__printf(1, 5) struct workqueue_struct *
+alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
+			    struct lockdep_map *lockdep_map, ...);
+
+/**
+ * alloc_ordered_workqueue_lockdep_map - allocate an ordered workqueue with
+ * user-defined lockdep_map
+ *
+ * @fmt: printf format for the name of the workqueue
+ * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
+ * @lockdep_map: user-defined lockdep_map
+ * @args: args for @fmt
+ *
+ * Same as alloc_ordered_workqueue but with the a user-define lockdep_map.
+ * Useful for workqueues created with the same purpose and to avoid leaking a
+ * lockdep_map on each workqueue creation.
+ *
+ * RETURNS:
+ * Pointer to the allocated workqueue on success, %NULL on failure.
+ */
+__printf(1, 4) static inline struct workqueue_struct *
+alloc_ordered_workqueue_lockdep_map(const char *fmt, unsigned int flags,
+				    struct lockdep_map *lockdep_map, ...)
+{
+	struct workqueue_struct *wq;
+	va_list args;
+
+	va_start(args, lockdep_map);
+	wq = alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | flags,
+					 1, lockdep_map, args);
+	va_end(args);
+
+	return wq;
+}
+#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 24df85589dc0..f4b50a995e99 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4775,11 +4775,17 @@ static void wq_init_lockdep(struct workqueue_struct *wq)
 
 static void wq_unregister_lockdep(struct workqueue_struct *wq)
 {
+	if (wq->lockdep_map != &wq->__lockdep_map)
+		return;
+
 	lockdep_unregister_key(&wq->key);
 }
 
 static void wq_free_lockdep(struct workqueue_struct *wq)
 {
+	if (wq->lockdep_map != &wq->__lockdep_map)
+		return;
+
 	if (wq->lock_name != wq->name)
 		kfree(wq->lock_name);
 }
@@ -5756,6 +5762,28 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 }
 EXPORT_SYMBOL_GPL(alloc_workqueue);
 
+#ifdef CONFIG_LOCKDEP
+__printf(1, 5)
+struct workqueue_struct *
+alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
+			    int max_active, struct lockdep_map *lockdep_map, ...)
+{
+	struct workqueue_struct *wq;
+	va_list args;
+
+	va_start(args, lockdep_map);
+	wq = __alloc_workqueue(fmt, flags, max_active, args);
+	va_end(args);
+	if (!wq)
+		return NULL;
+
+	wq->lockdep_map = lockdep_map;
+
+	return wq;
+}
+EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
+#endif
+
 static bool pwq_busy(struct pool_workqueue *pwq)
 {
 	int i;
-- 
2.34.1

Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Ghimiray, Himal Prasad]

On 10-08-2024 03:58, Matthew Brost wrote:
> Add an interface for a user-defined workqueue lockdep map, which is
> helpful when multiple workqueues are created for the same purpose. This
> also helps avoid leaking lockdep maps on each workqueue creation.
> 
> v2:
>   - Add alloc_workqueue_lockdep_map (Tejun)
> v3:
>   - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
>   - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> 
> 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 | 52 +++++++++++++++++++++++++++++++++++++++
>   kernel/workqueue.c        | 28 +++++++++++++++++++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 4eb8f9563136..8ccbf510880b 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -507,6 +507,58 @@ void workqueue_softirq_dead(unsigned int cpu);
>   __printf(1, 4) struct workqueue_struct *
>   alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
>   
> +#ifdef CONFIG_LOCKDEP
> +/**
> + * alloc_workqueue_lockdep_map - allocate a workqueue with user-defined lockdep_map
> + * @fmt: printf format for the name of the workqueue
> + * @flags: WQ_* flags
> + * @max_active: max in-flight work items, 0 for default
> + * @lockdep_map: user-defined lockdep_map
> + * @...: args for @fmt
> + *
> + * Same as alloc_workqueue but with the a user-define lockdep_map. Useful for
> + * workqueues created with the same purpose and to avoid leaking a lockdep_map
> + * on each workqueue creation.
> + *
> + * RETURNS:
> + * Pointer to the allocated workqueue on success, %NULL on failure.
> + */
> +__printf(1, 5) struct workqueue_struct *
> +alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> +			    struct lockdep_map *lockdep_map, ...);
> +
> +/**
> + * alloc_ordered_workqueue_lockdep_map - allocate an ordered workqueue with
> + * user-defined lockdep_map
> + *
> + * @fmt: printf format for the name of the workqueue
> + * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
> + * @lockdep_map: user-defined lockdep_map
> + * @args: args for @fmt
> + *
> + * Same as alloc_ordered_workqueue but with the a user-define lockdep_map.
> + * Useful for workqueues created with the same purpose and to avoid leaking a
> + * lockdep_map on each workqueue creation.
> + *
> + * RETURNS:
> + * Pointer to the allocated workqueue on success, %NULL on failure.
> + */
> +__printf(1, 4) static inline struct workqueue_struct *
> +alloc_ordered_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> +				    struct lockdep_map *lockdep_map, ...)
> +{
> +	struct workqueue_struct *wq;
> +	va_list args;
> +
> +	va_start(args, lockdep_map);
> +	wq = alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | flags,
> +					 1, lockdep_map, args);
> +	va_end(args);
> +
> +	return wq;
> +}
> +#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 24df85589dc0..f4b50a995e99 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4775,11 +4775,17 @@ static void wq_init_lockdep(struct workqueue_struct *wq)
>   
>   static void wq_unregister_lockdep(struct workqueue_struct *wq)
>   {
> +	if (wq->lockdep_map != &wq->__lockdep_map)
> +		return;
> +
>   	lockdep_unregister_key(&wq->key);
>   }
>   
>   static void wq_free_lockdep(struct workqueue_struct *wq)
>   {
> +	if (wq->lockdep_map != &wq->__lockdep_map)
> +		return;
> +
>   	if (wq->lock_name != wq->name)
>   		kfree(wq->lock_name);
>   }
> @@ -5756,6 +5762,28 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>   }
>   EXPORT_SYMBOL_GPL(alloc_workqueue);
>   
> +#ifdef CONFIG_LOCKDEP
> +__printf(1, 5)
> +struct workqueue_struct *
> +alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> +			    int max_active, struct lockdep_map *lockdep_map, ...)
> +{
> +	struct workqueue_struct *wq;
> +	va_list args;
> +
> +	va_start(args, lockdep_map);
> +	wq = __alloc_workqueue(fmt, flags, max_active, args);
> +	va_end(args);
> +	if (!wq)
> +		return NULL;
> +
> +	wq->lockdep_map = lockdep_map;

How about NULL check and fallback to dynamic allocation, just to be safe   ?

> +
> +	return wq;
> +}
> +EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
> +#endif
> +
>   static bool pwq_busy(struct pool_workqueue *pwq)
>   {
>   	int i;

Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Tejun Heo]

On Fri, Aug 09, 2024 at 03:28:25PM -0700, Matthew Brost wrote:
> Add an interface for a user-defined workqueue lockdep map, which is
> helpful when multiple workqueues are created for the same purpose. This
> also helps avoid leaking lockdep maps on each workqueue creation.
> 
> v2:
>  - Add alloc_workqueue_lockdep_map (Tejun)
> v3:
>  - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
>  - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>

1-3 look fine to me. Would applying them to wq/for-6.12 and pulling them
from the dri tree work?

Thanks.

-- 
tejun

Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Matthew Brost]

On Tue, Aug 13, 2024 at 08:52:26AM -1000, Tejun Heo wrote:
> On Fri, Aug 09, 2024 at 03:28:25PM -0700, Matthew Brost wrote:
> > Add an interface for a user-defined workqueue lockdep map, which is
> > helpful when multiple workqueues are created for the same purpose. This
> > also helps avoid leaking lockdep maps on each workqueue creation.
> > 
> > v2:
> >  - Add alloc_workqueue_lockdep_map (Tejun)
> > v3:
> >  - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
> >  - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> 
> 1-3 look fine to me. Would applying them to wq/for-6.12 and pulling them
> from the dri tree work?
> 

Yes, I wanted to get this into 6.12 as I believe we are removing
forceprobe for our driver and it would be good to have this in for that.

Any idea what this is about though [1]?

Matt

[1] https://patchwork.freedesktop.org/patch/607769/?series=136701&rev=3#comment_1105151

> Thanks.
> 
> -- 
> tejun

Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Tejun Heo]

On Tue, Aug 13, 2024 at 06:55:20PM +0000, Matthew Brost wrote:
> On Tue, Aug 13, 2024 at 08:52:26AM -1000, Tejun Heo wrote:
> > On Fri, Aug 09, 2024 at 03:28:25PM -0700, Matthew Brost wrote:
> > > Add an interface for a user-defined workqueue lockdep map, which is
> > > helpful when multiple workqueues are created for the same purpose. This
> > > also helps avoid leaking lockdep maps on each workqueue creation.
> > > 
> > > v2:
> > >  - Add alloc_workqueue_lockdep_map (Tejun)
> > > v3:
> > >  - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
> > >  - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> > > 
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > 1-3 look fine to me. Would applying them to wq/for-6.12 and pulling them
> > from the dri tree work?
> > 
> 
> Yes, I wanted to get this into 6.12 as I believe we are removing
> forceprobe for our driver and it would be good to have this in for that.
> 
> Any idea what this is about though [1]?

It looks like misattribution to me. I'll apply 1-3 to wq/for-6.12.

Thanks.

-- 
tejun

Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

[Tejun Heo]

On Fri, Aug 09, 2024 at 03:28:25PM -0700, Matthew Brost wrote:
> Add an interface for a user-defined workqueue lockdep map, which is
> helpful when multiple workqueues are created for the same purpose. This
> also helps avoid leaking lockdep maps on each workqueue creation.
> 
> v2:
>  - Add alloc_workqueue_lockdep_map (Tejun)
> v3:
>  - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
>  - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>

Applied 1-3 to wq/for-6.12.

Thanks.

-- 
tejun

[PATCH v3 4/5] drm/sched: Use drm sched lockdep map for submit_wq

[Matthew Brost]

Avoid leaking a lockdep map on each drm sched creation and destruction
by using a single lockdep map for all drm sched allocated submit_wq.

v2:
 - Use alloc_ordered_workqueue_lockdep_map (Tejun)

Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index ab53ab486fe6..cf79d348cb32 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -87,6 +87,12 @@
 #define CREATE_TRACE_POINTS
 #include "gpu_scheduler_trace.h"
 
+#ifdef CONFIG_LOCKDEP
+static struct lockdep_map drm_sched_lockdep_map = {
+	.name = "drm_sched_lockdep_map"
+};
+#endif
+
 #define to_drm_sched_job(sched_job)		\
 		container_of((sched_job), struct drm_sched_job, queue_node)
 
@@ -1272,7 +1278,12 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		sched->submit_wq = submit_wq;
 		sched->own_submit_wq = false;
 	} else {
+#ifdef CONFIG_LOCKDEP
+		sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name, 0,
+								       &drm_sched_lockdep_map);
+#else
 		sched->submit_wq = alloc_ordered_workqueue(name, 0);
+#endif
 		if (!sched->submit_wq)
 			return -ENOMEM;
 
-- 
2.34.1

Re: [PATCH v3 4/5] drm/sched: Use drm sched lockdep map for submit_wq

[Ghimiray, Himal Prasad]

On 10-08-2024 03:58, Matthew Brost wrote:
> Avoid leaking a lockdep map on each drm sched creation and destruction
> by using a single lockdep map for all drm sched allocated submit_wq.
> 
> v2:
>   - Use alloc_ordered_workqueue_lockdep_map (Tejun)
> 
> Cc: Luben Tuikov <ltuikov89@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index ab53ab486fe6..cf79d348cb32 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -87,6 +87,12 @@
>   #define CREATE_TRACE_POINTS
>   #include "gpu_scheduler_trace.h"
>   
> +#ifdef CONFIG_LOCKDEP
> +static struct lockdep_map drm_sched_lockdep_map = {
> +	.name = "drm_sched_lockdep_map"
> +};


will it be better to use STATIC_LOCKDEP_MAP_INIT ? Initializing key here 
instead of while registering the class ?


> +#endif
> +
>   #define to_drm_sched_job(sched_job)		\
>   		container_of((sched_job), struct drm_sched_job, queue_node)
>   
> @@ -1272,7 +1278,12 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   		sched->submit_wq = submit_wq;
>   		sched->own_submit_wq = false;
>   	} else {
> +#ifdef CONFIG_LOCKDEP
> +		sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name, 0,
> +								       &drm_sched_lockdep_map);
> +#else
>   		sched->submit_wq = alloc_ordered_workqueue(name, 0);
> +#endif
>   		if (!sched->submit_wq)
>   			return -ENOMEM;
>   

Re: [PATCH v3 4/5] drm/sched: Use drm sched lockdep map for submit_wq

[Matthew Brost]

On Mon, Aug 12, 2024 at 11:05:31AM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 10-08-2024 03:58, Matthew Brost wrote:
> > Avoid leaking a lockdep map on each drm sched creation and destruction
> > by using a single lockdep map for all drm sched allocated submit_wq.
> > 
> > v2:
> >   - Use alloc_ordered_workqueue_lockdep_map (Tejun)
> > 
> > Cc: Luben Tuikov <ltuikov89@gmail.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/scheduler/sched_main.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index ab53ab486fe6..cf79d348cb32 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -87,6 +87,12 @@
> >   #define CREATE_TRACE_POINTS
> >   #include "gpu_scheduler_trace.h"
> > +#ifdef CONFIG_LOCKDEP
> > +static struct lockdep_map drm_sched_lockdep_map = {
> > +	.name = "drm_sched_lockdep_map"
> > +};
> 
> 
> will it be better to use STATIC_LOCKDEP_MAP_INIT ? Initializing key here
> instead of while registering the class ?
> 

Most existing design patterns in the kernel define static lockdep class
this way so I think this is fine. But honestly don't really have an
opinion here.

Matt

> 
> > +#endif
> > +
> >   #define to_drm_sched_job(sched_job)		\
> >   		container_of((sched_job), struct drm_sched_job, queue_node)
> > @@ -1272,7 +1278,12 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
> >   		sched->submit_wq = submit_wq;
> >   		sched->own_submit_wq = false;
> >   	} else {
> > +#ifdef CONFIG_LOCKDEP
> > +		sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name, 0,
> > +								       &drm_sched_lockdep_map);
> > +#else
> >   		sched->submit_wq = alloc_ordered_workqueue(name, 0);
> > +#endif
> >   		if (!sched->submit_wq)
> >   			return -ENOMEM;

Re: [PATCH v3 4/5] drm/sched: Use drm sched lockdep map for submit_wq

[Ghimiray, Himal Prasad]

On 14-08-2024 00:46, Matthew Brost wrote:
> On Mon, Aug 12, 2024 at 11:05:31AM +0530, Ghimiray, Himal Prasad wrote:
>>
>>
>> On 10-08-2024 03:58, Matthew Brost wrote:
>>> Avoid leaking a lockdep map on each drm sched creation and destruction
>>> by using a single lockdep map for all drm sched allocated submit_wq.
>>>
>>> v2:
>>>    - Use alloc_ordered_workqueue_lockdep_map (Tejun)
>>>
>>> Cc: Luben Tuikov <ltuikov89@gmail.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>    drivers/gpu/drm/scheduler/sched_main.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>> index ab53ab486fe6..cf79d348cb32 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -87,6 +87,12 @@
>>>    #define CREATE_TRACE_POINTS
>>>    #include "gpu_scheduler_trace.h"
>>> +#ifdef CONFIG_LOCKDEP
>>> +static struct lockdep_map drm_sched_lockdep_map = {
>>> +	.name = "drm_sched_lockdep_map"
>>> +};
>>
>>
>> will it be better to use STATIC_LOCKDEP_MAP_INIT ? Initializing key here
>> instead of while registering the class ?
>>
> 
> Most existing design patterns in the kernel define static lockdep class
> this way so I think this is fine. But honestly don't really have an
> opinion here.
> 
> Matt

In that case, I have no concerns with the current initialization.


> 
>>
>>> +#endif
>>> +
>>>    #define to_drm_sched_job(sched_job)		\
>>>    		container_of((sched_job), struct drm_sched_job, queue_node)
>>> @@ -1272,7 +1278,12 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>>>    		sched->submit_wq = submit_wq;
>>>    		sched->own_submit_wq = false;
>>>    	} else {
>>> +#ifdef CONFIG_LOCKDEP
>>> +		sched->submit_wq = alloc_ordered_workqueue_lockdep_map(name, 0,
>>> +								       &drm_sched_lockdep_map);
>>> +#else
>>>    		sched->submit_wq = alloc_ordered_workqueue(name, 0);
>>> +#endif
>>>    		if (!sched->submit_wq)
>>>    			return -ENOMEM;

[PATCH v3 5/5] drm/xe: Drop GuC submit_wq pool

[Matthew Brost]

Now that drm sched uses a single lockdep map for all submit_wq, drop the
GuC submit_wq pool hack.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 60 +-----------------------------
 drivers/gpu/drm/xe/xe_guc_types.h  |  7 ----
 2 files changed, 1 insertion(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 460808507947..882cef3a10dc 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -224,64 +224,11 @@ static bool exec_queue_killed_or_banned_or_wedged(struct xe_exec_queue *q)
 		 EXEC_QUEUE_STATE_BANNED));
 }
 
-#ifdef CONFIG_PROVE_LOCKING
-static int alloc_submit_wq(struct xe_guc *guc)
-{
-	int i;
-
-	for (i = 0; i < NUM_SUBMIT_WQ; ++i) {
-		guc->submission_state.submit_wq_pool[i] =
-			alloc_ordered_workqueue("submit_wq", 0);
-		if (!guc->submission_state.submit_wq_pool[i])
-			goto err_free;
-	}
-
-	return 0;
-
-err_free:
-	while (i)
-		destroy_workqueue(guc->submission_state.submit_wq_pool[--i]);
-
-	return -ENOMEM;
-}
-
-static void free_submit_wq(struct xe_guc *guc)
-{
-	int i;
-
-	for (i = 0; i < NUM_SUBMIT_WQ; ++i)
-		destroy_workqueue(guc->submission_state.submit_wq_pool[i]);
-}
-
-static struct workqueue_struct *get_submit_wq(struct xe_guc *guc)
-{
-	int idx = guc->submission_state.submit_wq_idx++ % NUM_SUBMIT_WQ;
-
-	return guc->submission_state.submit_wq_pool[idx];
-}
-#else
-static int alloc_submit_wq(struct xe_guc *guc)
-{
-	return 0;
-}
-
-static void free_submit_wq(struct xe_guc *guc)
-{
-
-}
-
-static struct workqueue_struct *get_submit_wq(struct xe_guc *guc)
-{
-	return NULL;
-}
-#endif
-
 static void guc_submit_fini(struct drm_device *drm, void *arg)
 {
 	struct xe_guc *guc = arg;
 
 	xa_destroy(&guc->submission_state.exec_queue_lookup);
-	free_submit_wq(guc);
 }
 
 static void guc_submit_wedged_fini(struct drm_device *drm, void *arg)
@@ -337,10 +284,6 @@ int xe_guc_submit_init(struct xe_guc *guc, unsigned int num_ids)
 	if (err)
 		return err;
 
-	err = alloc_submit_wq(guc);
-	if (err)
-		return err;
-
 	gt->exec_queue_ops = &guc_exec_queue_ops;
 
 	xa_init(&guc->submission_state.exec_queue_lookup);
@@ -1445,8 +1388,7 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
 	timeout = (q->vm && xe_vm_in_lr_mode(q->vm)) ? MAX_SCHEDULE_TIMEOUT :
 		  msecs_to_jiffies(q->sched_props.job_timeout_ms);
 	err = xe_sched_init(&ge->sched, &drm_sched_ops, &xe_sched_ops,
-			    get_submit_wq(guc),
-			    q->lrc[0]->ring.size / MAX_JOB_SIZE_BYTES, 64,
+			    NULL, q->lrc[0]->ring.size / MAX_JOB_SIZE_BYTES, 64,
 			    timeout, guc_to_gt(guc)->ordered_wq, NULL,
 			    q->name, gt_to_xe(q->gt)->drm.dev);
 	if (err)
diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
index 546ac6350a31..585f5c274f09 100644
--- a/drivers/gpu/drm/xe/xe_guc_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_types.h
@@ -72,13 +72,6 @@ struct xe_guc {
 		atomic_t stopped;
 		/** @submission_state.lock: protects submission state */
 		struct mutex lock;
-#ifdef CONFIG_PROVE_LOCKING
-#define NUM_SUBMIT_WQ	256
-		/** @submission_state.submit_wq_pool: submission ordered workqueues pool */
-		struct workqueue_struct *submit_wq_pool[NUM_SUBMIT_WQ];
-		/** @submission_state.submit_wq_idx: submission ordered workqueue index */
-		int submit_wq_idx;
-#endif
 		/** @submission_state.enabled: submission is enabled */
 		bool enabled;
 	} submission_state;
-- 
2.34.1