| Message ID | 20171129154145.26755-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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(-)