[PATCH 1/2] lockdep: finer-grained completion key for kthread

[PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

Ideally we'd create the key through a macro at the real callers and
pass it all the way down. This would give us better coverage for cases
where a bunch of kthreads are created for the same thing.
But this gets the job done meanwhile and unblocks our CI. Refining
later on is always possible.

v2:
- make it compile
- use the right map (Tvrtko)

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 kernel/kthread.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index cd50e99202b0..8df5fedb9529 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -44,6 +44,10 @@ struct kthread {
 	unsigned long flags;
 	unsigned int cpu;
 	void *data;
+#ifdef CONFIG_LOCKDEP_COMPLETIONS
+	struct lock_class_key parked_key;
+	struct lock_class_key exited_key;
+#endif
 	struct completion parked;
 	struct completion exited;
 #ifdef CONFIG_BLK_CGROUP
@@ -221,8 +225,17 @@ static int kthread(void *_create)
 	}
 
 	self->data = data;
-	init_completion(&self->exited);
-	init_completion(&self->parked);
+	/* these two completions are shared with all kthread, which is bonghist
+	 * imo */
+	lockdep_init_map_crosslock(&self->exited.map.map,
+			"(kthread completion)->exited",
+			&self->exited_key, 0);
+	init_completion_map(&self->exited, &self->exited.map.map);
+	lockdep_init_map_crosslock(&self->parked.map.map,
+			"(kthread completion)->parked",
+			&self->parked_key, 0);
+	init_completion_map(&self->parked, &self->exited.map.map);
+
 	current->vfork_done = &self->exited;
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] lockdep: Up MAX_LOCKDEP_CHAINS

[Daniel Vetter]

cross-release ftl

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=103707
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 kernel/locking/lockdep_internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h
index d459d624ba2a..41630a5385c6 100644
--- a/kernel/locking/lockdep_internals.h
+++ b/kernel/locking/lockdep_internals.h
@@ -69,7 +69,7 @@ enum {
 #else
 #define MAX_LOCKDEP_ENTRIES	32768UL
 
-#define MAX_LOCKDEP_CHAINS_BITS	16
+#define MAX_LOCKDEP_CHAINS_BITS	17
 
 /*
  * Stack-trace: tightly packed array of stack backtrace
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] lockdep: Up MAX_LOCKDEP_CHAINS

[Chris Wilson]

Quoting Daniel Vetter (2017-11-28 17:07:07)
> cross-release ftl

Fwiw, this isn't cross-release but us reloading the module many times,
creating a whole host of new lockclasses. Even more fun is when the
module gets a slightly different address and the new lock address hashes
into an old lock...

I did think about a module-hook to revoke the stale lockclasses, but
that still leaves all the hashed chains.

This particular nuisance was temporarily pushed back by teaching igt not
to reload i915.ko on a whim.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] lockdep: finer-grained completion key for kthread

[Patchwork]

== Series Details ==

Series: series starting with [1/2] lockdep: finer-grained completion key for kthread
URL   : https://patchwork.freedesktop.org/series/34556/
State : success

== Summary ==

Series 34556v1 series starting with [1/2] lockdep: finer-grained completion key for kthread
https://patchwork.freedesktop.org/api/1.0/series/34556/revisions/1/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:421s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:427s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:454s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:253s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:471s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:470s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:443s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:427s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:409s
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:240s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:514s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:374s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:246s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:409s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:464s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:460s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:513s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:482s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:433s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:525s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:547s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:505s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:544s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:600s
fi-cnl-y         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:583s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:461s
fi-blb-e6850 failed to collect. IGT log at Patchwork_7328/fi-blb-e6850/igt.log
fi-kbl-7560u failed to collect. IGT log at Patchwork_7328/fi-kbl-7560u/igt.log

5144438448829ec2a3d94fd16a9e69a52cfa7b3b drm-tip: 2017y-11m-28d-17h-04m-56s UTC integration manifest
77504fcd19ba lockdep: Up MAX_LOCKDEP_CHAINS
0dc70bf6d898 lockdep: finer-grained completion key for kthread

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7328/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] lockdep: Up MAX_LOCKDEP_CHAINS

[Daniel Vetter]

On Tue, Nov 28, 2017 at 05:22:00PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-11-28 17:07:07)
> > cross-release ftl
> 
> Fwiw, this isn't cross-release but us reloading the module many times,
> creating a whole host of new lockclasses. Even more fun is when the
> module gets a slightly different address and the new lock address hashes
> into an old lock...
> 
> I did think about a module-hook to revoke the stale lockclasses, but
> that still leaves all the hashed chains.
> 
> This particular nuisance was temporarily pushed back by teaching igt not
> to reload i915.ko on a whim.

Ah ... Added your explanation to the commit message, and I guess that just
means we'll have to carry it ourselves :-/

I'll still send them out if CI approves to lockdep folks, just as an fyi.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] lockdep: finer-grained completion key for kthread

[Patchwork]

== Series Details ==

Series: series starting with [1/2] lockdep: finer-grained completion key for kthread
URL   : https://patchwork.freedesktop.org/series/34556/
State : success

== Summary ==

Blacklisted hosts:
shard-hsw        total:2661 pass:1532 dwarn:2   dfail:0   fail:11  skip:1116 time:8786s
shard-snb        total:2559 pass:1238 dwarn:12  dfail:4   fail:7   skip:1296 time:7820s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7328/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

On Wed, Nov 29, 2017 at 04:41:44PM +0100, Daniel Vetter wrote:
> Ideally we'd create the key through a macro at the real callers and
> pass it all the way down. This would give us better coverage for cases
> where a bunch of kthreads are created for the same thing.
> But this gets the job done meanwhile and unblocks our CI. Refining
> later on is always possible.
> 
> v2:
> - make it compile
> - use the right map (Tvrtko)
> 
> v3:
> 
> lockdep insist on a static key, so the cheap way didn't work. Wire
> 2 keys through all the callers.
> 
> I didn't extend this up to alloc_workqueue because the
> lockdep_invariant_state() call should separate the work functions from
> the workqueue kthread logic and prevent cross-release state from
> leaking between unrelated work queues that happen to reuse the same
> kthreads.
> 
> v4: CI found more compile fail :-/
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Any comments on this issue here? Is there a real patch we should use
instead of this quick hack?

As mentioned in the cover letter, I don't really have much clue, this is
more an elaborate bug report than anything like a real code submission :-)

Thanks, Daniel

> ---
>  include/linux/kthread.h | 48 ++++++++++++++++++++++++++++-----
>  kernel/kthread.c        | 70 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 90 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index c1961761311d..7a9463f0be5c 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -6,10 +6,12 @@
>  #include <linux/sched.h>
>  #include <linux/cgroup.h>
>  
> -__printf(4, 5)
> -struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
> +__printf(6, 7)
> +struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
>  					   void *data,
>  					   int node,
> +					   struct lock_class_key *exited_key,
> +					   struct lock_class_key *parked_key,
>  					   const char namefmt[], ...);
>  
>  /**
> @@ -25,12 +27,27 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
>   */
>  #define kthread_create(threadfn, data, namefmt, arg...) \
>  	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
> +#define kthread_create_on_node(threadfn, data, node, namefmt, arg...)	\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_on_node(threadfn, data, node, &__exited_key,	\
> +			       &__parked_key, namefmt, ##arg);		\
> +})
>  
>  
> -struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> +struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
>  					  void *data,
>  					  unsigned int cpu,
> +					  struct lock_class_key *exited_key,
> +					  struct lock_class_key *parked_key,
>  					  const char *namefmt);
> +#define kthread_create_on_cpu(threadfn, data, cpu, namefmt)		\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_on_cpu(threadfn, data, cpu, &__exited_key,\
> +			       &__parked_key, namefmt);			\
> +})
> +
>  
>  /**
>   * kthread_run - create and wake a thread.
> @@ -171,13 +188,30 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
>  
>  int kthread_worker_fn(void *worker_ptr);
>  
> -__printf(2, 3)
> +__printf(4, 5)
>  struct kthread_worker *
> -kthread_create_worker(unsigned int flags, const char namefmt[], ...);
> +_kthread_create_worker(unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
> +		       const char namefmt[], ...);
> +#define kthread_create_worker(flags, namefmt...)				\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_worker(flags, &__exited_key, &__parked_key,	\
> +			       ##namefmt);				\
> +})
>  
> -__printf(3, 4) struct kthread_worker *
> -kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +__printf(5, 6) struct kthread_worker *
> +_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
>  			     const char namefmt[], ...);
> +#define kthread_create_worker_on_cpu(cpu, flags, namefmt...)	\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_worker_on_cpu(cpu, flags, &__exited_key, &__parked_key,\
> +			       ##namefmt);				\
> +})
>  
>  bool kthread_queue_work(struct kthread_worker *worker,
>  			struct kthread_work *work);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index cd50e99202b0..9022806818fc 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -32,6 +32,7 @@ struct kthread_create_info
>  	int (*threadfn)(void *data);
>  	void *data;
>  	int node;
> +	struct lock_class_key *exited_key, *parked_key;
>  
>  	/* Result passed back to kthread_create() from kthreadd. */
>  	struct task_struct *result;
> @@ -221,8 +222,17 @@ static int kthread(void *_create)
>  	}
>  
>  	self->data = data;
> -	init_completion(&self->exited);
> -	init_completion(&self->parked);
> +	/* these two completions are shared with all kthread, which is bonghist
> +	 * imo */
> +	lockdep_init_map_crosslock(&self->exited.map.map,
> +			"(kthread completion)->exited",
> +			create->exited_key, 0);
> +	init_completion_map(&self->exited, &self->exited.map.map);
> +	lockdep_init_map_crosslock(&self->parked.map.map,
> +			"(kthread completion)->parked",
> +			create->parked_key, 0);
> +	init_completion_map(&self->parked, &self->exited.map.map);
> +
>  	current->vfork_done = &self->exited;
>  
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
> @@ -272,9 +282,11 @@ static void create_kthread(struct kthread_create_info *create)
>  	}
>  }
>  
> -static __printf(4, 0)
> +static __printf(6, 0)
>  struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  						    void *data, int node,
> +						    struct lock_class_key *exited_key,
> +						    struct lock_class_key *parked_key,
>  						    const char namefmt[],
>  						    va_list args)
>  {
> @@ -289,6 +301,8 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  	create->data = data;
>  	create->node = node;
>  	create->done = &done;
> +	create->exited_key = exited_key;
> +	create->parked_key = parked_key;
>  
>  	spin_lock(&kthread_create_lock);
>  	list_add_tail(&create->list, &kthread_create_list);
> @@ -353,21 +367,24 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>   *
>   * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
>   */
> -struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
> -					   void *data, int node,
> -					   const char namefmt[],
> -					   ...)
> +struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
> +					    void *data, int node,
> +					    struct lock_class_key *exited_key,
> +					    struct lock_class_key *parked_key,
> +					    const char namefmt[],
> +					    ...)
>  {
>  	struct task_struct *task;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
> +	task = __kthread_create_on_node(threadfn, data, node,
> +					exited_key, parked_key, namefmt, args);
>  	va_end(args);
>  
>  	return task;
>  }
> -EXPORT_SYMBOL(kthread_create_on_node);
> +EXPORT_SYMBOL(_kthread_create_on_node);
>  
>  static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)
>  {
> @@ -421,14 +438,16 @@ EXPORT_SYMBOL(kthread_bind);
>   * Description: This helper function creates and names a kernel thread
>   * The thread will be woken and put into park mode.
>   */
> -struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> +struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
>  					  void *data, unsigned int cpu,
> +					  struct lock_class_key *exited_key,
> +					  struct lock_class_key *parked_key,
>  					  const char *namefmt)
>  {
>  	struct task_struct *p;
>  
> -	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
> -				   cpu);
> +	p = _kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
> +				    exited_key, parked_key, namefmt, cpu);
>  	if (IS_ERR(p))
>  		return p;
>  	kthread_bind(p, cpu);
> @@ -649,8 +668,10 @@ int kthread_worker_fn(void *worker_ptr)
>  }
>  EXPORT_SYMBOL_GPL(kthread_worker_fn);
>  
> -static __printf(3, 0) struct kthread_worker *
> +static __printf(5, 0) struct kthread_worker *
>  __kthread_create_worker(int cpu, unsigned int flags,
> +			struct lock_class_key *exited_key,
> +			struct lock_class_key *parked_key,
>  			const char namefmt[], va_list args)
>  {
>  	struct kthread_worker *worker;
> @@ -666,8 +687,8 @@ __kthread_create_worker(int cpu, unsigned int flags,
>  	if (cpu >= 0)
>  		node = cpu_to_node(cpu);
>  
> -	task = __kthread_create_on_node(kthread_worker_fn, worker,
> -						node, namefmt, args);
> +	task = __kthread_create_on_node(kthread_worker_fn, worker, node,
> +					exited_key, parked_key, namefmt, args);
>  	if (IS_ERR(task))
>  		goto fail_task;
>  
> @@ -694,18 +715,22 @@ __kthread_create_worker(int cpu, unsigned int flags,
>   * when the worker was SIGKILLed.
>   */
>  struct kthread_worker *
> -kthread_create_worker(unsigned int flags, const char namefmt[], ...)
> +_kthread_create_worker(unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
> +		       const char namefmt[], ...)
>  {
>  	struct kthread_worker *worker;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	worker = __kthread_create_worker(-1, flags, namefmt, args);
> +	worker = __kthread_create_worker(-1, flags, exited_key, parked_key,
> +					 namefmt, args);
>  	va_end(args);
>  
>  	return worker;
>  }
> -EXPORT_SYMBOL(kthread_create_worker);
> +EXPORT_SYMBOL(_kthread_create_worker);
>  
>  /**
>   * kthread_create_worker_on_cpu - create a kthread worker and bind it
> @@ -725,19 +750,22 @@ EXPORT_SYMBOL(kthread_create_worker);
>   * when the worker was SIGKILLed.
>   */
>  struct kthread_worker *
> -kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +			      struct lock_class_key *exited_key,
> +			      struct lock_class_key *parked_key,
>  			     const char namefmt[], ...)
>  {
>  	struct kthread_worker *worker;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	worker = __kthread_create_worker(cpu, flags, namefmt, args);
> +	worker = __kthread_create_worker(cpu, flags, exited_key, parked_key,
> +					 namefmt, args);
>  	va_end(args);
>  
>  	return worker;
>  }
> -EXPORT_SYMBOL(kthread_create_worker_on_cpu);
> +EXPORT_SYMBOL(_kthread_create_worker_on_cpu);
>  
>  /*
>   * Returns true when the work could not be queued at the moment.
> -- 
> 2.15.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

On Wed, Nov 29, 2017 at 04:41:44PM +0100, Daniel Vetter wrote:
> Ideally we'd create the key through a macro at the real callers and
> pass it all the way down. This would give us better coverage for cases
> where a bunch of kthreads are created for the same thing.
> But this gets the job done meanwhile and unblocks our CI. Refining
> later on is always possible.
> 
> v2:
> - make it compile
> - use the right map (Tvrtko)
> 
> v3:
> 
> lockdep insist on a static key, so the cheap way didn't work. Wire
> 2 keys through all the callers.
> 
> I didn't extend this up to alloc_workqueue because the
> lockdep_invariant_state() call should separate the work functions from
> the workqueue kthread logic and prevent cross-release state from
> leaking between unrelated work queues that happen to reuse the same
> kthreads.
> 
> v4: CI found more compile fail :-/
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Any comments on this issue here? Is there a real patch we should use
instead of this quick hack?

As mentioned in the cover letter, I don't really have much clue, this is
more an elaborate bug report than anything like a real code submission :-)

Thanks, Daniel

> ---
>  include/linux/kthread.h | 48 ++++++++++++++++++++++++++++-----
>  kernel/kthread.c        | 70 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 90 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index c1961761311d..7a9463f0be5c 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -6,10 +6,12 @@
>  #include <linux/sched.h>
>  #include <linux/cgroup.h>
>  
> -__printf(4, 5)
> -struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
> +__printf(6, 7)
> +struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
>  					   void *data,
>  					   int node,
> +					   struct lock_class_key *exited_key,
> +					   struct lock_class_key *parked_key,
>  					   const char namefmt[], ...);
>  
>  /**
> @@ -25,12 +27,27 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
>   */
>  #define kthread_create(threadfn, data, namefmt, arg...) \
>  	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
> +#define kthread_create_on_node(threadfn, data, node, namefmt, arg...)	\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_on_node(threadfn, data, node, &__exited_key,	\
> +			       &__parked_key, namefmt, ##arg);		\
> +})
>  
>  
> -struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> +struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
>  					  void *data,
>  					  unsigned int cpu,
> +					  struct lock_class_key *exited_key,
> +					  struct lock_class_key *parked_key,
>  					  const char *namefmt);
> +#define kthread_create_on_cpu(threadfn, data, cpu, namefmt)		\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_on_cpu(threadfn, data, cpu, &__exited_key,\
> +			       &__parked_key, namefmt);			\
> +})
> +
>  
>  /**
>   * kthread_run - create and wake a thread.
> @@ -171,13 +188,30 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
>  
>  int kthread_worker_fn(void *worker_ptr);
>  
> -__printf(2, 3)
> +__printf(4, 5)
>  struct kthread_worker *
> -kthread_create_worker(unsigned int flags, const char namefmt[], ...);
> +_kthread_create_worker(unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
> +		       const char namefmt[], ...);
> +#define kthread_create_worker(flags, namefmt...)				\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_worker(flags, &__exited_key, &__parked_key,	\
> +			       ##namefmt);				\
> +})
>  
> -__printf(3, 4) struct kthread_worker *
> -kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +__printf(5, 6) struct kthread_worker *
> +_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
>  			     const char namefmt[], ...);
> +#define kthread_create_worker_on_cpu(cpu, flags, namefmt...)	\
> +({									\
> +	static struct lock_class_key __exited_key, __parked_key;	\
> +	_kthread_create_worker_on_cpu(cpu, flags, &__exited_key, &__parked_key,\
> +			       ##namefmt);				\
> +})
>  
>  bool kthread_queue_work(struct kthread_worker *worker,
>  			struct kthread_work *work);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index cd50e99202b0..9022806818fc 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -32,6 +32,7 @@ struct kthread_create_info
>  	int (*threadfn)(void *data);
>  	void *data;
>  	int node;
> +	struct lock_class_key *exited_key, *parked_key;
>  
>  	/* Result passed back to kthread_create() from kthreadd. */
>  	struct task_struct *result;
> @@ -221,8 +222,17 @@ static int kthread(void *_create)
>  	}
>  
>  	self->data = data;
> -	init_completion(&self->exited);
> -	init_completion(&self->parked);
> +	/* these two completions are shared with all kthread, which is bonghist
> +	 * imo */
> +	lockdep_init_map_crosslock(&self->exited.map.map,
> +			"(kthread completion)->exited",
> +			create->exited_key, 0);
> +	init_completion_map(&self->exited, &self->exited.map.map);
> +	lockdep_init_map_crosslock(&self->parked.map.map,
> +			"(kthread completion)->parked",
> +			create->parked_key, 0);
> +	init_completion_map(&self->parked, &self->exited.map.map);
> +
>  	current->vfork_done = &self->exited;
>  
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
> @@ -272,9 +282,11 @@ static void create_kthread(struct kthread_create_info *create)
>  	}
>  }
>  
> -static __printf(4, 0)
> +static __printf(6, 0)
>  struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  						    void *data, int node,
> +						    struct lock_class_key *exited_key,
> +						    struct lock_class_key *parked_key,
>  						    const char namefmt[],
>  						    va_list args)
>  {
> @@ -289,6 +301,8 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  	create->data = data;
>  	create->node = node;
>  	create->done = &done;
> +	create->exited_key = exited_key;
> +	create->parked_key = parked_key;
>  
>  	spin_lock(&kthread_create_lock);
>  	list_add_tail(&create->list, &kthread_create_list);
> @@ -353,21 +367,24 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>   *
>   * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
>   */
> -struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
> -					   void *data, int node,
> -					   const char namefmt[],
> -					   ...)
> +struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
> +					    void *data, int node,
> +					    struct lock_class_key *exited_key,
> +					    struct lock_class_key *parked_key,
> +					    const char namefmt[],
> +					    ...)
>  {
>  	struct task_struct *task;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
> +	task = __kthread_create_on_node(threadfn, data, node,
> +					exited_key, parked_key, namefmt, args);
>  	va_end(args);
>  
>  	return task;
>  }
> -EXPORT_SYMBOL(kthread_create_on_node);
> +EXPORT_SYMBOL(_kthread_create_on_node);
>  
>  static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)
>  {
> @@ -421,14 +438,16 @@ EXPORT_SYMBOL(kthread_bind);
>   * Description: This helper function creates and names a kernel thread
>   * The thread will be woken and put into park mode.
>   */
> -struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> +struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
>  					  void *data, unsigned int cpu,
> +					  struct lock_class_key *exited_key,
> +					  struct lock_class_key *parked_key,
>  					  const char *namefmt)
>  {
>  	struct task_struct *p;
>  
> -	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
> -				   cpu);
> +	p = _kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
> +				    exited_key, parked_key, namefmt, cpu);
>  	if (IS_ERR(p))
>  		return p;
>  	kthread_bind(p, cpu);
> @@ -649,8 +668,10 @@ int kthread_worker_fn(void *worker_ptr)
>  }
>  EXPORT_SYMBOL_GPL(kthread_worker_fn);
>  
> -static __printf(3, 0) struct kthread_worker *
> +static __printf(5, 0) struct kthread_worker *
>  __kthread_create_worker(int cpu, unsigned int flags,
> +			struct lock_class_key *exited_key,
> +			struct lock_class_key *parked_key,
>  			const char namefmt[], va_list args)
>  {
>  	struct kthread_worker *worker;
> @@ -666,8 +687,8 @@ __kthread_create_worker(int cpu, unsigned int flags,
>  	if (cpu >= 0)
>  		node = cpu_to_node(cpu);
>  
> -	task = __kthread_create_on_node(kthread_worker_fn, worker,
> -						node, namefmt, args);
> +	task = __kthread_create_on_node(kthread_worker_fn, worker, node,
> +					exited_key, parked_key, namefmt, args);
>  	if (IS_ERR(task))
>  		goto fail_task;
>  
> @@ -694,18 +715,22 @@ __kthread_create_worker(int cpu, unsigned int flags,
>   * when the worker was SIGKILLed.
>   */
>  struct kthread_worker *
> -kthread_create_worker(unsigned int flags, const char namefmt[], ...)
> +_kthread_create_worker(unsigned int flags,
> +		       struct lock_class_key *exited_key,
> +		       struct lock_class_key *parked_key,
> +		       const char namefmt[], ...)
>  {
>  	struct kthread_worker *worker;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	worker = __kthread_create_worker(-1, flags, namefmt, args);
> +	worker = __kthread_create_worker(-1, flags, exited_key, parked_key,
> +					 namefmt, args);
>  	va_end(args);
>  
>  	return worker;
>  }
> -EXPORT_SYMBOL(kthread_create_worker);
> +EXPORT_SYMBOL(_kthread_create_worker);
>  
>  /**
>   * kthread_create_worker_on_cpu - create a kthread worker and bind it
> @@ -725,19 +750,22 @@ EXPORT_SYMBOL(kthread_create_worker);
>   * when the worker was SIGKILLed.
>   */
>  struct kthread_worker *
> -kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
> +			      struct lock_class_key *exited_key,
> +			      struct lock_class_key *parked_key,
>  			     const char namefmt[], ...)
>  {
>  	struct kthread_worker *worker;
>  	va_list args;
>  
>  	va_start(args, namefmt);
> -	worker = __kthread_create_worker(cpu, flags, namefmt, args);
> +	worker = __kthread_create_worker(cpu, flags, exited_key, parked_key,
> +					 namefmt, args);
>  	va_end(args);
>  
>  	return worker;
>  }
> -EXPORT_SYMBOL(kthread_create_worker_on_cpu);
> +EXPORT_SYMBOL(_kthread_create_worker_on_cpu);
>  
>  /*
>   * Returns true when the work could not be queued at the moment.
> -- 
> 2.15.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

Ideally we'd create the key through a macro at the real callers and
pass it all the way down. This would give us better coverage for cases
where a bunch of kthreads are created for the same thing.
But this gets the job done meanwhile and unblocks our CI. Refining
later on is always possible.

v2:
- make it compile
- use the right map (Tvrtko)

v3:

lockdep insist on a static key, so the cheap way didn't work. Wire
2 keys through all the callers.

I didn't extend this up to alloc_workqueue because the
lockdep_invariant_state() call should separate the work functions from
the workqueue kthread logic and prevent cross-release state from
leaking between unrelated work queues that happen to reuse the same
kthreads.

v4: CI found more compile fail :-/

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/kthread.h | 48 ++++++++++++++++++++++++++++-----
 kernel/kthread.c        | 70 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 90 insertions(+), 28 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index c1961761311d..7a9463f0be5c 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -6,10 +6,12 @@
 #include <linux/sched.h>
 #include <linux/cgroup.h>
 
-__printf(4, 5)
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
+__printf(6, 7)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
 					   void *data,
 					   int node,
+					   struct lock_class_key *exited_key,
+					   struct lock_class_key *parked_key,
 					   const char namefmt[], ...);
 
 /**
@@ -25,12 +27,27 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
  */
 #define kthread_create(threadfn, data, namefmt, arg...) \
 	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
+#define kthread_create_on_node(threadfn, data, node, namefmt, arg...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_node(threadfn, data, node, &__exited_key,	\
+			       &__parked_key, namefmt, ##arg);		\
+})
 
 
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data,
 					  unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt);
+#define kthread_create_on_cpu(threadfn, data, cpu, namefmt)		\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_cpu(threadfn, data, cpu, &__exited_key,\
+			       &__parked_key, namefmt);			\
+})
+
 
 /**
  * kthread_run - create and wake a thread.
@@ -171,13 +188,30 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
 
 int kthread_worker_fn(void *worker_ptr);
 
-__printf(2, 3)
+__printf(4, 5)
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...);
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...);
+#define kthread_create_worker(flags, namefmt...)				\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker(flags, &__exited_key, &__parked_key,	\
+			       ##namefmt);				\
+})
 
-__printf(3, 4) struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+__printf(5, 6) struct kthread_worker *
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
 			     const char namefmt[], ...);
+#define kthread_create_worker_on_cpu(cpu, flags, namefmt...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker_on_cpu(cpu, flags, &__exited_key, &__parked_key,\
+			       ##namefmt);				\
+})
 
 bool kthread_queue_work(struct kthread_worker *worker,
 			struct kthread_work *work);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index cd50e99202b0..9022806818fc 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -32,6 +32,7 @@ struct kthread_create_info
 	int (*threadfn)(void *data);
 	void *data;
 	int node;
+	struct lock_class_key *exited_key, *parked_key;
 
 	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
@@ -221,8 +222,17 @@ static int kthread(void *_create)
 	}
 
 	self->data = data;
-	init_completion(&self->exited);
-	init_completion(&self->parked);
+	/* these two completions are shared with all kthread, which is bonghist
+	 * imo */
+	lockdep_init_map_crosslock(&self->exited.map.map,
+			"(kthread completion)->exited",
+			create->exited_key, 0);
+	init_completion_map(&self->exited, &self->exited.map.map);
+	lockdep_init_map_crosslock(&self->parked.map.map,
+			"(kthread completion)->parked",
+			create->parked_key, 0);
+	init_completion_map(&self->parked, &self->exited.map.map);
+
 	current->vfork_done = &self->exited;
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
@@ -272,9 +282,11 @@ static void create_kthread(struct kthread_create_info *create)
 	}
 }
 
-static __printf(4, 0)
+static __printf(6, 0)
 struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 						    void *data, int node,
+						    struct lock_class_key *exited_key,
+						    struct lock_class_key *parked_key,
 						    const char namefmt[],
 						    va_list args)
 {
@@ -289,6 +301,8 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	create->data = data;
 	create->node = node;
 	create->done = &done;
+	create->exited_key = exited_key;
+	create->parked_key = parked_key;
 
 	spin_lock(&kthread_create_lock);
 	list_add_tail(&create->list, &kthread_create_list);
@@ -353,21 +367,24 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
  *
  * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
  */
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
-					   void *data, int node,
-					   const char namefmt[],
-					   ...)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
+					    void *data, int node,
+					    struct lock_class_key *exited_key,
+					    struct lock_class_key *parked_key,
+					    const char namefmt[],
+					    ...)
 {
 	struct task_struct *task;
 	va_list args;
 
 	va_start(args, namefmt);
-	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
+	task = __kthread_create_on_node(threadfn, data, node,
+					exited_key, parked_key, namefmt, args);
 	va_end(args);
 
 	return task;
 }
-EXPORT_SYMBOL(kthread_create_on_node);
+EXPORT_SYMBOL(_kthread_create_on_node);
 
 static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)
 {
@@ -421,14 +438,16 @@ EXPORT_SYMBOL(kthread_bind);
  * Description: This helper function creates and names a kernel thread
  * The thread will be woken and put into park mode.
  */
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data, unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt)
 {
 	struct task_struct *p;
 
-	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
-				   cpu);
+	p = _kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
+				    exited_key, parked_key, namefmt, cpu);
 	if (IS_ERR(p))
 		return p;
 	kthread_bind(p, cpu);
@@ -649,8 +668,10 @@ int kthread_worker_fn(void *worker_ptr)
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
-static __printf(3, 0) struct kthread_worker *
+static __printf(5, 0) struct kthread_worker *
 __kthread_create_worker(int cpu, unsigned int flags,
+			struct lock_class_key *exited_key,
+			struct lock_class_key *parked_key,
 			const char namefmt[], va_list args)
 {
 	struct kthread_worker *worker;
@@ -666,8 +687,8 @@ __kthread_create_worker(int cpu, unsigned int flags,
 	if (cpu >= 0)
 		node = cpu_to_node(cpu);
 
-	task = __kthread_create_on_node(kthread_worker_fn, worker,
-						node, namefmt, args);
+	task = __kthread_create_on_node(kthread_worker_fn, worker, node,
+					exited_key, parked_key, namefmt, args);
 	if (IS_ERR(task))
 		goto fail_task;
 
@@ -694,18 +715,22 @@ __kthread_create_worker(int cpu, unsigned int flags,
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...)
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(-1, flags, namefmt, args);
+	worker = __kthread_create_worker(-1, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker);
+EXPORT_SYMBOL(_kthread_create_worker);
 
 /**
  * kthread_create_worker_on_cpu - create a kthread worker and bind it
@@ -725,19 +750,22 @@ EXPORT_SYMBOL(kthread_create_worker);
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+			      struct lock_class_key *exited_key,
+			      struct lock_class_key *parked_key,
 			     const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(cpu, flags, namefmt, args);
+	worker = __kthread_create_worker(cpu, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker_on_cpu);
+EXPORT_SYMBOL(_kthread_create_worker_on_cpu);
 
 /*
  * Returns true when the work could not be queued at the moment.
-- 
2.15.0

[PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

Ideally we'd create the key through a macro at the real callers and
pass it all the way down. This would give us better coverage for cases
where a bunch of kthreads are created for the same thing.
But this gets the job done meanwhile and unblocks our CI. Refining
later on is always possible.

v2:
- make it compile
- use the right map (Tvrtko)

v3:

lockdep insist on a static key, so the cheap way didn't work. Wire
2 keys through all the callers.

I didn't extend this up to alloc_workqueue because the
lockdep_invariant_state() call should separate the work functions from
the workqueue kthread logic and prevent cross-release state from
leaking between unrelated work queues that happen to reuse the same
kthreads.

v4: CI found more compile fail :-/

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/kthread.h | 48 ++++++++++++++++++++++++++++-----
 kernel/kthread.c        | 70 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 90 insertions(+), 28 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index c1961761311d..7a9463f0be5c 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -6,10 +6,12 @@
 #include <linux/sched.h>
 #include <linux/cgroup.h>
 
-__printf(4, 5)
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
+__printf(6, 7)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
 					   void *data,
 					   int node,
+					   struct lock_class_key *exited_key,
+					   struct lock_class_key *parked_key,
 					   const char namefmt[], ...);
 
 /**
@@ -25,12 +27,27 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
  */
 #define kthread_create(threadfn, data, namefmt, arg...) \
 	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
+#define kthread_create_on_node(threadfn, data, node, namefmt, arg...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_node(threadfn, data, node, &__exited_key,	\
+			       &__parked_key, namefmt, ##arg);		\
+})
 
 
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data,
 					  unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt);
+#define kthread_create_on_cpu(threadfn, data, cpu, namefmt)		\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_cpu(threadfn, data, cpu, &__exited_key,\
+			       &__parked_key, namefmt);			\
+})
+
 
 /**
  * kthread_run - create and wake a thread.
@@ -171,13 +188,30 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
 
 int kthread_worker_fn(void *worker_ptr);
 
-__printf(2, 3)
+__printf(4, 5)
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...);
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...);
+#define kthread_create_worker(flags, namefmt...)				\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker(flags, &__exited_key, &__parked_key,	\
+			       ##namefmt);				\
+})
 
-__printf(3, 4) struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+__printf(5, 6) struct kthread_worker *
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
 			     const char namefmt[], ...);
+#define kthread_create_worker_on_cpu(cpu, flags, namefmt...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker_on_cpu(cpu, flags, &__exited_key, &__parked_key,\
+			       ##namefmt);				\
+})
 
 bool kthread_queue_work(struct kthread_worker *worker,
 			struct kthread_work *work);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index cd50e99202b0..9022806818fc 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -32,6 +32,7 @@ struct kthread_create_info
 	int (*threadfn)(void *data);
 	void *data;
 	int node;
+	struct lock_class_key *exited_key, *parked_key;
 
 	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
@@ -221,8 +222,17 @@ static int kthread(void *_create)
 	}
 
 	self->data = data;
-	init_completion(&self->exited);
-	init_completion(&self->parked);
+	/* these two completions are shared with all kthread, which is bonghist
+	 * imo */
+	lockdep_init_map_crosslock(&self->exited.map.map,
+			"(kthread completion)->exited",
+			create->exited_key, 0);
+	init_completion_map(&self->exited, &self->exited.map.map);
+	lockdep_init_map_crosslock(&self->parked.map.map,
+			"(kthread completion)->parked",
+			create->parked_key, 0);
+	init_completion_map(&self->parked, &self->exited.map.map);
+
 	current->vfork_done = &self->exited;
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
@@ -272,9 +282,11 @@ static void create_kthread(struct kthread_create_info *create)
 	}
 }
 
-static __printf(4, 0)
+static __printf(6, 0)
 struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 						    void *data, int node,
+						    struct lock_class_key *exited_key,
+						    struct lock_class_key *parked_key,
 						    const char namefmt[],
 						    va_list args)
 {
@@ -289,6 +301,8 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	create->data = data;
 	create->node = node;
 	create->done = &done;
+	create->exited_key = exited_key;
+	create->parked_key = parked_key;
 
 	spin_lock(&kthread_create_lock);
 	list_add_tail(&create->list, &kthread_create_list);
@@ -353,21 +367,24 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
  *
  * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
  */
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
-					   void *data, int node,
-					   const char namefmt[],
-					   ...)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
+					    void *data, int node,
+					    struct lock_class_key *exited_key,
+					    struct lock_class_key *parked_key,
+					    const char namefmt[],
+					    ...)
 {
 	struct task_struct *task;
 	va_list args;
 
 	va_start(args, namefmt);
-	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
+	task = __kthread_create_on_node(threadfn, data, node,
+					exited_key, parked_key, namefmt, args);
 	va_end(args);
 
 	return task;
 }
-EXPORT_SYMBOL(kthread_create_on_node);
+EXPORT_SYMBOL(_kthread_create_on_node);
 
 static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)
 {
@@ -421,14 +438,16 @@ EXPORT_SYMBOL(kthread_bind);
  * Description: This helper function creates and names a kernel thread
  * The thread will be woken and put into park mode.
  */
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data, unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt)
 {
 	struct task_struct *p;
 
-	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
-				   cpu);
+	p = _kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
+				    exited_key, parked_key, namefmt, cpu);
 	if (IS_ERR(p))
 		return p;
 	kthread_bind(p, cpu);
@@ -649,8 +668,10 @@ int kthread_worker_fn(void *worker_ptr)
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
-static __printf(3, 0) struct kthread_worker *
+static __printf(5, 0) struct kthread_worker *
 __kthread_create_worker(int cpu, unsigned int flags,
+			struct lock_class_key *exited_key,
+			struct lock_class_key *parked_key,
 			const char namefmt[], va_list args)
 {
 	struct kthread_worker *worker;
@@ -666,8 +687,8 @@ __kthread_create_worker(int cpu, unsigned int flags,
 	if (cpu >= 0)
 		node = cpu_to_node(cpu);
 
-	task = __kthread_create_on_node(kthread_worker_fn, worker,
-						node, namefmt, args);
+	task = __kthread_create_on_node(kthread_worker_fn, worker, node,
+					exited_key, parked_key, namefmt, args);
 	if (IS_ERR(task))
 		goto fail_task;
 
@@ -694,18 +715,22 @@ __kthread_create_worker(int cpu, unsigned int flags,
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...)
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(-1, flags, namefmt, args);
+	worker = __kthread_create_worker(-1, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker);
+EXPORT_SYMBOL(_kthread_create_worker);
 
 /**
  * kthread_create_worker_on_cpu - create a kthread worker and bind it
@@ -725,19 +750,22 @@ EXPORT_SYMBOL(kthread_create_worker);
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+			      struct lock_class_key *exited_key,
+			      struct lock_class_key *parked_key,
 			     const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(cpu, flags, namefmt, args);
+	worker = __kthread_create_worker(cpu, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker_on_cpu);
+EXPORT_SYMBOL(_kthread_create_worker_on_cpu);
 
 /*
  * Returns true when the work could not be queued at the moment.
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] lockdep: finer-grained completion key for kthread

[Daniel Vetter]

Ideally we'd create the key through a macro at the real callers and
pass it all the way down. This would give us better coverage for cases
where a bunch of kthreads are created for the same thing.
But this gets the job done meanwhile and unblocks our CI. Refining
later on is always possible.

v2:
- make it compile
- use the right map (Tvrtko)

v3:

lockdep insist on a static key, so the cheap way didn't work. Wire
2 keys through all the callers.

I didn't extend this up to alloc_workqueue because the
lockdep_invariant_state() call should separate the work functions from
the workqueue kthread logic and prevent cross-release state from
leaking between unrelated work queues that happen to reuse the same
kthreads.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=103950
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/kthread.h | 48 ++++++++++++++++++++++++++++-----
 kernel/kthread.c        | 70 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 90 insertions(+), 28 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index c1961761311d..3763a9657928 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -6,10 +6,12 @@
 #include <linux/sched.h>
 #include <linux/cgroup.h>
 
-__printf(4, 5)
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
+__printf(6, 7)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
 					   void *data,
 					   int node,
+					   struct lock_class_key *exited_key,
+					   struct lock_class_key *parked_key,
 					   const char namefmt[], ...);
 
 /**
@@ -25,12 +27,27 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
  */
 #define kthread_create(threadfn, data, namefmt, arg...) \
 	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
+#define kthread_create_on_node(threadfn, data, node, namefmt, arg...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_node(threadfn, data, node, &__exited_key,	\
+			       &__parked_key, namefmt, ##arg);		\
+})
 
 
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data,
 					  unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt);
+#define kthread_create_on_cpu(threadfn, data, cpu, namefmt)		\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_on_cpu(threadfn, data, cpu, &__exited_key,\
+			       &__parked_key, namefmt);			\
+})
+
 
 /**
  * kthread_run - create and wake a thread.
@@ -171,13 +188,30 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
 
 int kthread_worker_fn(void *worker_ptr);
 
-__printf(2, 3)
+__printf(4, 5)
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...);
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...);
+#define kthread_create_worker(flags, namefmt...)				\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker(flags, &__exited_key, &__parked_key,	\
+			       ##namefmt);				\
+})
 
-__printf(3, 4) struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+__printf(5, 6) struct kthread_worker *
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
 			     const char namefmt[], ...);
+#define kthread_create_worker_on_cpu(cpu, flags, namefmt...)	\
+({									\
+	static struct lock_class_key __exited_key, __parked_key;	\
+	_kthread_create_worker(cpu, flags, &__exited_key, &__parked_key,\
+			       ##namefmt);				\
+})
 
 bool kthread_queue_work(struct kthread_worker *worker,
 			struct kthread_work *work);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index cd50e99202b0..9022806818fc 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -32,6 +32,7 @@ struct kthread_create_info
 	int (*threadfn)(void *data);
 	void *data;
 	int node;
+	struct lock_class_key *exited_key, *parked_key;
 
 	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
@@ -221,8 +222,17 @@ static int kthread(void *_create)
 	}
 
 	self->data = data;
-	init_completion(&self->exited);
-	init_completion(&self->parked);
+	/* these two completions are shared with all kthread, which is bonghist
+	 * imo */
+	lockdep_init_map_crosslock(&self->exited.map.map,
+			"(kthread completion)->exited",
+			create->exited_key, 0);
+	init_completion_map(&self->exited, &self->exited.map.map);
+	lockdep_init_map_crosslock(&self->parked.map.map,
+			"(kthread completion)->parked",
+			create->parked_key, 0);
+	init_completion_map(&self->parked, &self->exited.map.map);
+
 	current->vfork_done = &self->exited;
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
@@ -272,9 +282,11 @@ static void create_kthread(struct kthread_create_info *create)
 	}
 }
 
-static __printf(4, 0)
+static __printf(6, 0)
 struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 						    void *data, int node,
+						    struct lock_class_key *exited_key,
+						    struct lock_class_key *parked_key,
 						    const char namefmt[],
 						    va_list args)
 {
@@ -289,6 +301,8 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	create->data = data;
 	create->node = node;
 	create->done = &done;
+	create->exited_key = exited_key;
+	create->parked_key = parked_key;
 
 	spin_lock(&kthread_create_lock);
 	list_add_tail(&create->list, &kthread_create_list);
@@ -353,21 +367,24 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
  *
  * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
  */
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
-					   void *data, int node,
-					   const char namefmt[],
-					   ...)
+struct task_struct *_kthread_create_on_node(int (*threadfn)(void *data),
+					    void *data, int node,
+					    struct lock_class_key *exited_key,
+					    struct lock_class_key *parked_key,
+					    const char namefmt[],
+					    ...)
 {
 	struct task_struct *task;
 	va_list args;
 
 	va_start(args, namefmt);
-	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
+	task = __kthread_create_on_node(threadfn, data, node,
+					exited_key, parked_key, namefmt, args);
 	va_end(args);
 
 	return task;
 }
-EXPORT_SYMBOL(kthread_create_on_node);
+EXPORT_SYMBOL(_kthread_create_on_node);
 
 static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)
 {
@@ -421,14 +438,16 @@ EXPORT_SYMBOL(kthread_bind);
  * Description: This helper function creates and names a kernel thread
  * The thread will be woken and put into park mode.
  */
-struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
+struct task_struct *_kthread_create_on_cpu(int (*threadfn)(void *data),
 					  void *data, unsigned int cpu,
+					  struct lock_class_key *exited_key,
+					  struct lock_class_key *parked_key,
 					  const char *namefmt)
 {
 	struct task_struct *p;
 
-	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
-				   cpu);
+	p = _kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
+				    exited_key, parked_key, namefmt, cpu);
 	if (IS_ERR(p))
 		return p;
 	kthread_bind(p, cpu);
@@ -649,8 +668,10 @@ int kthread_worker_fn(void *worker_ptr)
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
-static __printf(3, 0) struct kthread_worker *
+static __printf(5, 0) struct kthread_worker *
 __kthread_create_worker(int cpu, unsigned int flags,
+			struct lock_class_key *exited_key,
+			struct lock_class_key *parked_key,
 			const char namefmt[], va_list args)
 {
 	struct kthread_worker *worker;
@@ -666,8 +687,8 @@ __kthread_create_worker(int cpu, unsigned int flags,
 	if (cpu >= 0)
 		node = cpu_to_node(cpu);
 
-	task = __kthread_create_on_node(kthread_worker_fn, worker,
-						node, namefmt, args);
+	task = __kthread_create_on_node(kthread_worker_fn, worker, node,
+					exited_key, parked_key, namefmt, args);
 	if (IS_ERR(task))
 		goto fail_task;
 
@@ -694,18 +715,22 @@ __kthread_create_worker(int cpu, unsigned int flags,
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker(unsigned int flags, const char namefmt[], ...)
+_kthread_create_worker(unsigned int flags,
+		       struct lock_class_key *exited_key,
+		       struct lock_class_key *parked_key,
+		       const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(-1, flags, namefmt, args);
+	worker = __kthread_create_worker(-1, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker);
+EXPORT_SYMBOL(_kthread_create_worker);
 
 /**
  * kthread_create_worker_on_cpu - create a kthread worker and bind it
@@ -725,19 +750,22 @@ EXPORT_SYMBOL(kthread_create_worker);
  * when the worker was SIGKILLed.
  */
 struct kthread_worker *
-kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+_kthread_create_worker_on_cpu(int cpu, unsigned int flags,
+			      struct lock_class_key *exited_key,
+			      struct lock_class_key *parked_key,
 			     const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __kthread_create_worker(cpu, flags, namefmt, args);
+	worker = __kthread_create_worker(cpu, flags, exited_key, parked_key,
+					 namefmt, args);
 	va_end(args);
 
 	return worker;
 }
-EXPORT_SYMBOL(kthread_create_worker_on_cpu);
+EXPORT_SYMBOL(_kthread_create_worker_on_cpu);
 
 /*
  * Returns true when the work could not be queued at the moment.
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx