[PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Andy Walls]

init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
a dynamically allocated kthread_worker object's internal spinlock_t.
This causes lockdep to gripe:

	INFO: trying to register non-static key.
	the code is fine but needs lockdep annotation.
	turning off the locking correctness validator.

To keep lockdep happy, use spin_lock_init() for dynamically
allocated kthread_worker objects' internal spinlock_t.

Reported-by: Nicolas <nicolas.mailhot@laposte.net>
Signed-off-by: Andy Walls <awalls@md.metrocast.net>

Cc: Tejun Heo <tj@kernel.org>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 685ea65..e65d0b1 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -83,7 +83,13 @@ struct kthread_work {
 
 static inline void init_kthread_worker(struct kthread_worker *worker)
 {
-	*worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
+	/*
+	 * Lockdep complains if a dynamically allocated worker's spinlock_t
+	 * is initialzed using SPIN_LOCK_UNLOCKED.
+	 */
+	spin_lock_init(&worker->lock);
+	INIT_LIST_HEAD(&worker->work_list);
+	worker->task = NULL;
 }
 
 static inline void init_kthread_work(struct kthread_work *work,

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Yong Zhang]

On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
> initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
> a dynamically allocated kthread_worker object's internal spinlock_t.
> This causes lockdep to gripe:
>
>        INFO: trying to register non-static key.
>        the code is fine but needs lockdep annotation.
>        turning off the locking correctness validator.
>
> To keep lockdep happy, use spin_lock_init() for dynamically
> allocated kthread_worker objects' internal spinlock_t.
>
> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jarod Wilson <jarod@redhat.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 685ea65..e65d0b1 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -83,7 +83,13 @@ struct kthread_work {
>
>  static inline void init_kthread_worker(struct kthread_worker *worker)
>  {
> -       *worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
> +       /*
> +        * Lockdep complains if a dynamically allocated worker's spinlock_t
> +        * is initialzed using SPIN_LOCK_UNLOCKED.
> +        */
> +       spin_lock_init(&worker->lock);

This will make different kthead_worker->lock initialized with one same
key. You know spin_lock_init() will be like below:
# define raw_spin_lock_init(lock)				\
do {								\
	static struct lock_class_key __key;			\
								\
	__raw_spin_lock_init((lock), #lock, &__key);		\
} while (0)

So we should put the real initializer to kernel/kthread.c
and make init_kthread_worker() to be a MACRO.

BTW, init_kthread_work() should also be changed like above
because member done is a wait_queue_head.

Thanks,
Yong

-- 
Only stand for myself.

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Yong Zhang]

On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>> init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
>> initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
>> a dynamically allocated kthread_worker object's internal spinlock_t.
>> This causes lockdep to gripe:
>>
>>        INFO: trying to register non-static key.
>>        the code is fine but needs lockdep annotation.
>>        turning off the locking correctness validator.
>>
>> To keep lockdep happy, use spin_lock_init() for dynamically
>> allocated kthread_worker objects' internal spinlock_t.
>>
>> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
>> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Jarod Wilson <jarod@redhat.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>>
>> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
>> index 685ea65..e65d0b1 100644
>> --- a/include/linux/kthread.h
>> +++ b/include/linux/kthread.h
>> @@ -83,7 +83,13 @@ struct kthread_work {
>>
>>  static inline void init_kthread_worker(struct kthread_worker *worker)
>>  {
>> -       *worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
>> +       /*
>> +        * Lockdep complains if a dynamically allocated worker's spinlock_t
>> +        * is initialzed using SPIN_LOCK_UNLOCKED.
>> +        */
>> +       spin_lock_init(&worker->lock);
>
> This will make different kthead_worker->lock initialized with one same
> key. You know spin_lock_init() will be like below:
> # define raw_spin_lock_init(lock)                               \
> do {                                                            \
>        static struct lock_class_key __key;                     \
>                                                                \
>        __raw_spin_lock_init((lock), #lock, &__key);            \
> } while (0)
>
> So we should put the real initializer to kernel/kthread.c
> and make init_kthread_worker() to be a MACRO.
>
> BTW, init_kthread_work() should also be changed like above
> because member done is a wait_queue_head.

untested patch is here. Andy/Nicolas, is it ok for you?
---
Subject: [PATCH] kthread_work: Make lockdep happy

spinlock in kthread_worker and wait_queue_head in kthread_work
both should be lockdep annotated.
So change the interface to make it suiltable for CONFIG_LOCKDEP.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
I'm not sure if it's possible to define a worker on stack?
So I left DEFINE_KTHREAD_WORKER() untouched.

 include/linux/kthread.h |   37 +++++++++++++++++++++++++++----------
 kernel/kthread.c        |    9 +++++++++
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 685ea65..5d516b3 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -75,22 +75,39 @@ struct kthread_work {
 	.flushing = ATOMIC_INIT(0),					\
 	}

+/* Is it possible to define a worker on stack? */
 #define DEFINE_KTHREAD_WORKER(worker)					\
 	struct kthread_worker worker = KTHREAD_WORKER_INIT(worker)

 #define DEFINE_KTHREAD_WORK(work, fn)					\
 	struct kthread_work work = KTHREAD_WORK_INIT(work, fn)

-static inline void init_kthread_worker(struct kthread_worker *worker)
-{
-	*worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
-}
-
-static inline void init_kthread_work(struct kthread_work *work,
-				     kthread_work_func_t fn)
-{
-	*work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
-}
+#ifdef CONFIG_LOCKDEP
+# define KTHREAD_WORK_INIT_ONSTACK(work, fn)				\
+	({init_kthread_work((&work), fn); work})
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn)				\
+	struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
+#else
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
+#endif
+
+extern void __init_kthread_worker(struct kthread_worker *worker,
+					struct lock_class_key *key);
+
+#define init_kthread_worker(worker)					\
+	do {								\
+		static struct lock_class_key __key;			\
+		__init_kthread_worker((worker), &__key);		\
+	} while (0)
+
+#define init_kthread_work(work, fn)					\
+	do {								\
+		memset((work), 0, sizeof(struct kthread_work));		\
+		INIT_LIST_HEAD(&(work)->node);				\
+		(work)->func = (fn);					\
+		init_waitqueue_head(&(work)->done);			\
+		(work)->flushing = ATOMIC_INIT(0);			\
+	} while (0)

 int kthread_worker_fn(void *worker_ptr);

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 2dc3786..fae2eff 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -265,6 +265,15 @@ int kthreadd(void *unused)
 	return 0;
 }

+void __init_kthread_worker(struct kthread_worker *worker,
+				struct lock_class_key *key)
+{
+	spin_lock_init(&worker->lock);
+	lockdep_set_class(&worker->lock, key);
+	INIT_LIST_HEAD(&worker->work_list);
+	worker->task == NULL;
+}
+
 /**
  * kthread_worker_fn - kthread function to process kthread_worker
  * @worker_ptr: pointer to initialized kthread_worker
-- 
1.7.0.4
-- 
Only stand for myself.

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Tejun Heo]

Hello,

On 12/20/2010 10:28 AM, Yong Zhang wrote:
> Subject: [PATCH] kthread_work: Make lockdep happy
> 
> spinlock in kthread_worker and wait_queue_head in kthread_work
> both should be lockdep annotated.
> So change the interface to make it suiltable for CONFIG_LOCKDEP.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> ---
> I'm not sure if it's possible to define a worker on stack?
> So I left DEFINE_KTHREAD_WORKER() untouched.

Yes, it can, I think.  BTW, where are you using kthread_worker?  I'm
planning to update its flush semantics similar to that of proper
workqueue so that it's less confusing and switching between the two is
easy, so its usage may change slightly soon, although conversion
shouldn't be difficult.

Thanks.

-- 
tejun

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Yong Zhang]

On Tue, Dec 21, 2010 at 12:21 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 12/20/2010 10:28 AM, Yong Zhang wrote:
>> Subject: [PATCH] kthread_work: Make lockdep happy
>>
>> spinlock in kthread_worker and wait_queue_head in kthread_work
>> both should be lockdep annotated.
>> So change the interface to make it suiltable for CONFIG_LOCKDEP.
>>
>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>> ---
>> I'm not sure if it's possible to define a worker on stack?
>> So I left DEFINE_KTHREAD_WORKER() untouched.
>
> Yes, it can, I think.

Thanks for your confirmation, I will update the patch in V2.

>BTW, where are you using kthread_worker?

I don't have anything using kthread_worker, but go through
the kernel and find ivtv is the only one.
This is why I sent the untested patch :)

> I'm
> planning to update its flush semantics similar to that of proper
> workqueue so that it's less confusing and switching between the two is
> easy, so its usage may change slightly soon, although conversion
> shouldn't be difficult.

No problem. Now I just make the patch based on the current
kthread_work*.

Thanks,
Yong

-- 
Only stand for myself.

[V2 PATCH] kthread_work: Make lockdep happy

[Yong Zhang]

From: Yong Zhang <yong.zhang0@gmail.com>
Subject: [V2 PATCH] kthread_work: Make lockdep happy

spinlock in kthread_worker and wait_queue_head in kthread_work
both should be lockdep sensible.
So change the interface to make it suiltable for CONFIG_LOCKDEP.

Reported-by: Nicolas <nicolas.mailhot@laposte.net>
Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Walls <awalls@md.metrocast.net>
---
Changes from V1:
 *According to Tejun, kthread_worker could be defined on stack,
  So introduce DEFINE_KTHREAD_WORKER_ONSTACK.
 *Change wrong setting to kthread_work->task. Thanks Adny for
  pointing it.
 *including some minor issue.

BTW, only passed build.

 include/linux/kthread.h |   45 +++++++++++++++++++++++++++++++++++----------
 kernel/kthread.c        |    9 +++++++++
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 685ea65..f8b3320 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -81,16 +81,41 @@ struct kthread_work {
 #define DEFINE_KTHREAD_WORK(work, fn)					\
 	struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
 
-static inline void init_kthread_worker(struct kthread_worker *worker)
-{
-	*worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
-}
-
-static inline void init_kthread_work(struct kthread_work *work,
-				     kthread_work_func_t fn)
-{
-	*work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
-}
+/*
+ * kthread_worker.lock and kthread_work.done need
+ * special work if they are defined on stack with
+ * lockdep enabled.
+ */
+#ifdef CONFIG_LOCKDEP
+# define KTHREAD_WORKER_INIT_ONSTACK(worker)				\
+	({ init_kthread_worker(&worker); worker; })
+# define DEFINE_KTHREAD_WORKER_ONSTACK(worker)				\
+	struct kthread_worker worker = KTHREAD_WORKER_INIT_ONSTACK(worker)
+# define KTHREAD_WORK_INIT_ONSTACK(work, fn)				\
+	({ init_kthread_work((&work), fn); work; })
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn)				\
+	struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
+#else
+# define DEFINE_KTHREAD_WORKER_ONSTACK(worker) DEFINE_KTHREAD_WORKER(worker)
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
+#endif
+
+extern void __init_kthread_worker(struct kthread_worker *worker,
+					struct lock_class_key *key);
+
+#define init_kthread_worker(worker)					\
+	do {								\
+		static struct lock_class_key __key;			\
+		__init_kthread_worker((worker), &__key);		\
+	} while (0)
+
+#define init_kthread_work(work, fn)					\
+	do {								\
+		memset((work), 0, sizeof(struct kthread_work));		\
+		INIT_LIST_HEAD(&(work)->node);				\
+		(work)->func = (fn);					\
+		init_waitqueue_head(&(work)->done);			\
+	} while (0)
 
 int kthread_worker_fn(void *worker_ptr);
 
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 2dc3786..f760587 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -265,6 +265,15 @@ int kthreadd(void *unused)
 	return 0;
 }
 
+void __init_kthread_worker(struct kthread_worker *worker,
+				struct lock_class_key *key)
+{
+	spin_lock_init(&worker->lock);
+	lockdep_set_class(&worker->lock, key);
+	INIT_LIST_HEAD(&worker->work_list);
+	worker->task = NULL;
+}
+
 /**
  * kthread_worker_fn - kthread function to process kthread_worker
  * @worker_ptr: pointer to initialized kthread_worker
-- 
1.7.0.4

Re: [V2 PATCH] kthread_work: Make lockdep happy

[Tejun Heo]

Hello,

On Tue, Dec 21, 2010 at 12:40:50PM +0800, Yong Zhang wrote:
> From: Yong Zhang <yong.zhang0@gmail.com>
> Subject: [V2 PATCH] kthread_work: Make lockdep happy
> 
> spinlock in kthread_worker and wait_queue_head in kthread_work
> both should be lockdep sensible.
> So change the interface to make it suiltable for CONFIG_LOCKDEP.
> 
> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Walls <awalls@md.metrocast.net>
> ---
> Changes from V1:
>  *According to Tejun, kthread_worker could be defined on stack,
>   So introduce DEFINE_KTHREAD_WORKER_ONSTACK.
>  *Change wrong setting to kthread_work->task. Thanks Adny for
>   pointing it.
>  *including some minor issue.
> 
> BTW, only passed build.

If somebody can confirm this makes lockdep behave correctly, I'll
route it through the wq tree.

Thanks.

-- 
tejun

Re: [V2 PATCH] kthread_work: Make lockdep happy

[Nicolas Mailhot]

Le Mar 21 décembre 2010 13:59, Tejun Heo a écrit :

> If somebody can confirm this makes lockdep behave correctly, I'll
> route it through the wq tree.

I can boot-test it if someone adds it to the rawhide kernel or a kernel that
can be downloaded from koji (don't really have the time to build kernels right
now)

-- 
Nicolas Mailhot

Re: [V2 PATCH] kthread_work: Make lockdep happy

[Andy Walls]

On Tue, 2010-12-21 at 13:59 +0100, Tejun Heo wrote:
> Hello,
> 
> On Tue, Dec 21, 2010 at 12:40:50PM +0800, Yong Zhang wrote:
> > From: Yong Zhang <yong.zhang0@gmail.com>
> > Subject: [V2 PATCH] kthread_work: Make lockdep happy
> > 
> > spinlock in kthread_worker and wait_queue_head in kthread_work
> > both should be lockdep sensible.
> > So change the interface to make it suiltable for CONFIG_LOCKDEP.
> > 
> > Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> > Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Walls <awalls@md.metrocast.net>
> > ---
> > Changes from V1:
> >  *According to Tejun, kthread_worker could be defined on stack,
> >   So introduce DEFINE_KTHREAD_WORKER_ONSTACK.
> >  *Change wrong setting to kthread_work->task. Thanks Adny for
> >   pointing it.
> >  *including some minor issue.
> > 
> > BTW, only passed build.
> 
> If somebody can confirm this makes lockdep behave correctly, I'll
> route it through the wq tree.

I will attempt to test later tonight with at least one ivtv (PVR-350)
card installed.  If I have time, I'll test with two cards (PVR-350 and
PVR-150) installed in the machine, since that will create two different
kthread_workers, each with their own lock.

How can I dump information on lockdep spinlock tracking to verify that
the two distinct locks are tracked separately by lockdep?

Regards,
Andy

Re: [V2 PATCH] kthread_work: Make lockdep happy

[Andy Walls]

On Tue, 2010-12-21 at 12:40 +0800, Yong Zhang wrote:
> From: Yong Zhang <yong.zhang0@gmail.com>
> Subject: [V2 PATCH] kthread_work: Make lockdep happy
> 
> spinlock in kthread_worker and wait_queue_head in kthread_work
> both should be lockdep sensible.
> So change the interface to make it suiltable for CONFIG_LOCKDEP.
> 
> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Walls <awalls@md.metrocast.net>
> ---
> Changes from V1:
>  *According to Tejun, kthread_worker could be defined on stack,
>   So introduce DEFINE_KTHREAD_WORKER_ONSTACK.
>  *Change wrong setting to kthread_work->task. Thanks Adny for
>   pointing it.
>  *including some minor issue.
> 
> BTW, only passed build.
> 
>  include/linux/kthread.h |   45 +++++++++++++++++++++++++++++++++++----------
>  kernel/kthread.c        |    9 +++++++++
>  2 files changed, 44 insertions(+), 10 deletions(-)

Yong,

I have tested your patch with the following patch on top of yours.  I
find the two patches together acceptable in testing in a machine with
both a PVR-350 card and a PVR-500 card installed.

Tested-by: Andy Walls <awalls@md.metrocast.net>
Signed-off-by: Andy Walls <awalls@md.metrocast.net>

Could you please author a [V3 PATCH] adding my changes?  Since the
majority of the change is your work, it should be attributed to you as
the author.


About my additional changes:

I needed the explicit EXPORT_SYMBOL_GPL(__init_kthread_worker),
otherwise the build would fail, because the ivtv module had an
unresolved symbol.

I added the lockdep class name to make it easier to identify the ivtv
module's kthread worker's lock class.  Now one can actually recognize
the lock class in lockdep output strings:

	# cat lockdep | grep itv
	ffffffffa0483a60 FD:    1 BD:    1 ......: (&itv->irq_worker)->lock

Thanks again.

Regards,
Andy


diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index f8b3320..994564a 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -100,13 +100,15 @@ struct kthread_work {
 # define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
 #endif
 
-extern void __init_kthread_worker(struct kthread_worker *worker,
-					struct lock_class_key *key);
+void __init_kthread_worker(struct kthread_worker *worker,
+			   struct lock_class_key *key,
+			   const char *lock_class_name);
 
 #define init_kthread_worker(worker)					\
 	do {								\
 		static struct lock_class_key __key;			\
-		__init_kthread_worker((worker), &__key);		\
+		__init_kthread_worker((worker), &__key,			\
+				      "(" #worker ")->lock");		\
 	} while (0)
 
 #define init_kthread_work(work, fn)					\
diff --git a/kernel/kthread.c b/kernel/kthread.c
index f760587..4657ebd 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -266,13 +266,15 @@ int kthreadd(void *unused)
 }
 
 void __init_kthread_worker(struct kthread_worker *worker,
-				struct lock_class_key *key)
+			   struct lock_class_key *key,
+			   const char *lock_class_name)
 {
 	spin_lock_init(&worker->lock);
-	lockdep_set_class(&worker->lock, key);
+	lockdep_set_class_and_name(&worker->lock, key, lock_class_name);
 	INIT_LIST_HEAD(&worker->work_list);
 	worker->task = NULL;
 }
+EXPORT_SYMBOL_GPL(__init_kthread_worker);
 
 /**
  * kthread_worker_fn - kthread function to process kthread_worker

Re: [V2 PATCH] kthread_work: Make lockdep happy

[Yong Zhang]

On Wed, Dec 22, 2010 at 8:39 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> Yong,
>
> I have tested your patch with the following patch on top of yours.  I
> find the two patches together acceptable in testing in a machine with
> both a PVR-350 card and a PVR-500 card installed.
>
> Tested-by: Andy Walls <awalls@md.metrocast.net>
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>
> Could you please author a [V3 PATCH] adding my changes?

Sure. Will do it.

> Since the
> majority of the change is your work, it should be attributed to you as
> the author.
>
>
> About my additional changes:
>
> I needed the explicit EXPORT_SYMBOL_GPL(__init_kthread_worker),
> otherwise the build would fail, because the ivtv module had an
> unresolved symbol.

Will update.

>
> I added the lockdep class name to make it easier to identify the ivtv
> module's kthread worker's lock class.  Now one can actually recognize
> the lock class in lockdep output strings:
>
>        # cat lockdep | grep itv
>        ffffffffa0483a60 FD:    1 BD:    1 ......: (&itv->irq_worker)->lock

Yeah, I like your idea.

V3 coming soon :)

Thanks,
Yong
-- 
Only stand for myself.

[V3 PATCH] kthread_work: Make lockdep happy

[Yong Zhang]

From: Yong Zhang <yong.zhang0@gmail.com>
Subject: [V3 PATCH] kthread_work: Make lockdep happy

spinlock in kthread_worker and wait_queue_head in kthread_work
both should be lockdep sensible.
So change the interface to make it suiltable for CONFIG_LOCKDEP.

Reported-by: Nicolas <nicolas.mailhot@laposte.net>
Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
Tested-by: Andy Walls <awalls@md.metrocast.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Cahgnes form V2:
 *export __init_kthread_worker, fix module build issue.
 *explicitly name worker->lock, which will make debug more easy.
  idea from Andy Walls.

Changes from V1:
 *According to Tejun, kthread_worker could be defined on stack,
  So introduce DEFINE_KTHREAD_WORKER_ONSTACK.
 *Change wrong setting to kthread_work->task. Thanks Adny for
  pointing it.
 *including some minor issue.

 include/linux/kthread.h |   45 +++++++++++++++++++++++++++++++++++----------
 kernel/kthread.c        |   11 +++++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 685ea65..fa48dfc 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -81,16 +81,41 @@ struct kthread_work {
 #define DEFINE_KTHREAD_WORK(work, fn)					\
 	struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
 
-static inline void init_kthread_worker(struct kthread_worker *worker)
-{
-	*worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
-}
-
-static inline void init_kthread_work(struct kthread_work *work,
-				     kthread_work_func_t fn)
-{
-	*work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
-}
+/*
+ * kthread_worker.lock and kthread_work.done need
+ * special work if they are defined on stack with
+ * lockdep enabled.
+ */
+#ifdef CONFIG_LOCKDEP
+# define KTHREAD_WORKER_INIT_ONSTACK(worker)				\
+	({ init_kthread_worker(&worker); worker; })
+# define DEFINE_KTHREAD_WORKER_ONSTACK(worker)				\
+	struct kthread_worker worker = KTHREAD_WORKER_INIT_ONSTACK(worker)
+# define KTHREAD_WORK_INIT_ONSTACK(work, fn)				\
+	({ init_kthread_work((&work), fn); work; })
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn)				\
+	struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
+#else
+# define DEFINE_KTHREAD_WORKER_ONSTACK(worker) DEFINE_KTHREAD_WORKER(worker)
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
+#endif
+
+extern void __init_kthread_worker(struct kthread_worker *worker,
+			const char *name, struct lock_class_key *key);
+
+#define init_kthread_worker(worker)					\
+	do {								\
+		static struct lock_class_key __key;			\
+		__init_kthread_worker((worker), "("#worker")->lock", &__key); \
+	} while (0)
+
+#define init_kthread_work(work, fn)					\
+	do {								\
+		memset((work), 0, sizeof(struct kthread_work));		\
+		INIT_LIST_HEAD(&(work)->node);				\
+		(work)->func = (fn);					\
+		init_waitqueue_head(&(work)->done);			\
+	} while (0)
 
 int kthread_worker_fn(void *worker_ptr);
 
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 2dc3786..ca61bbd 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -265,6 +265,17 @@ int kthreadd(void *unused)
 	return 0;
 }
 
+void __init_kthread_worker(struct kthread_worker *worker,
+				const char *name,
+				struct lock_class_key *key)
+{
+	spin_lock_init(&worker->lock);
+	lockdep_set_class_and_name(&worker->lock, key, name);
+	INIT_LIST_HEAD(&worker->work_list);
+	worker->task = NULL;
+}
+EXPORT_SYMBOL_GPL(__init_kthread_worker);
+
 /**
  * kthread_worker_fn - kthread function to process kthread_worker
  * @worker_ptr: pointer to initialized kthread_worker
-- 
1.7.0.4

Re: [V3 PATCH] kthread_work: Make lockdep happy

[Tejun Heo]

On Wed, Dec 22, 2010 at 11:23:05AM +0800, Yong Zhang wrote:
> From: Yong Zhang <yong.zhang0@gmail.com>
> Subject: [V3 PATCH] kthread_work: Make lockdep happy
> 
> spinlock in kthread_worker and wait_queue_head in kthread_work
> both should be lockdep sensible.
> So change the interface to make it suiltable for CONFIG_LOCKDEP.
> 
> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> Tested-by: Andy Walls <awalls@md.metrocast.net>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>

Applied with the following comment update (because the term "special
work" was a bit too confusing given the context) to wq#for-linus.
Will push out to Linus in several days.

+/*
+ * kthread_worker.lock and kthread_work.done need their own lockdep class
+ * keys if they are defined on stack with lockdep enabled.  Use the
+ * following macros when defining them on stack.
+ */

Thank you.

-- 
tejun

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Andy Walls]

On Mon, 2010-12-20 at 17:28 +0800, Yong Zhang wrote:
> On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> > On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> >> init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
> >> initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
> >> a dynamically allocated kthread_worker object's internal spinlock_t.
> >> This causes lockdep to gripe:
> >>
> >>        INFO: trying to register non-static key.
> >>        the code is fine but needs lockdep annotation.
> >>        turning off the locking correctness validator.
> >>
> >> To keep lockdep happy, use spin_lock_init() for dynamically
> >> allocated kthread_worker objects' internal spinlock_t.
> >>
> >> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
> >> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> >>
> >> Cc: Tejun Heo <tj@kernel.org>
> >> Cc: Jarod Wilson <jarod@redhat.com>
> >> Cc: Ingo Molnar <mingo@redhat.com>
> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>

> > This will make different kthead_worker->lock initialized with one same
> > key.

Well, that wouldn't be very useful. :P


> > So we should put the real initializer to kernel/kthread.c
> > and make init_kthread_worker() to be a MACRO.

Sounds OK to me.  I'm not a lockdep expert and I made my initial patch
with the sole intention of making this bugzilla report go away:

https://bugzilla.redhat.com/show_bug.cgi?id=662384


> untested patch is here. Andy/Nicolas, is it ok for you?

No, see my comments below.

> ---
> Subject: [PATCH] kthread_work: Make lockdep happy
> 
> spinlock in kthread_worker and wait_queue_head in kthread_work
> both should be lockdep annotated.
> So change the interface to make it suiltable for CONFIG_LOCKDEP.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> ---
> I'm not sure if it's possible to define a worker on stack?
> So I left DEFINE_KTHREAD_WORKER() untouched.
> 
>  include/linux/kthread.h |   37 +++++++++++++++++++++++++++----------
>  kernel/kthread.c        |    9 +++++++++
>  2 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 685ea65..5d516b3 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -75,22 +75,39 @@ struct kthread_work {
>  	.flushing = ATOMIC_INIT(0),					\
>  	}
> 
> +/* Is it possible to define a worker on stack? */

This comment doesn't help a developer decide if this interface is OK to
use.

If there is an alternate preferred API for instantiating 1 (or more)
thread(s) to handle work objects off of the stack, then the comment
should point the reader to that API (e.g. singlethread_workqueue).

To answer the question in the comment:

It is possible to allocate a kthread worker off of the stack, but IMO it
has little advantage over a singlethread_workqueue allocated off of the
stack.

ivtv only needed the kthread_worker API, because it has some deferred
work with tight timing constraints.  ivtv sets the kthread_worker to
SCHED_FIFO scheduling for ivtv work, which couldn't be done on a
workqueue thread with the updated singlethread_workqueue implementation.
Note that ivtv does *not* allocate its kthread worker off of the stack.


>  #define DEFINE_KTHREAD_WORKER(worker)					\
>  	struct kthread_worker worker = KTHREAD_WORKER_INIT(worker)
> 
>  #define DEFINE_KTHREAD_WORK(work, fn)					\
>  	struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
> 
> -static inline void init_kthread_worker(struct kthread_worker *worker)
> -{
> -	*worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
> -}
> -
> -static inline void init_kthread_work(struct kthread_work *work,
> -				     kthread_work_func_t fn)
> -{
> -	*work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
> -}
> +#ifdef CONFIG_LOCKDEP
> +# define KTHREAD_WORK_INIT_ONSTACK(work, fn)				\
> +	({init_kthread_work((&work), fn); work})
> +# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn)				\
> +	struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
> +#else
> +# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
> +#endif
> +
> +extern void __init_kthread_worker(struct kthread_worker *worker,
> +					struct lock_class_key *key);
> +
> +#define init_kthread_worker(worker)					\
> +	do {								\
> +		static struct lock_class_key __key;			\
> +		__init_kthread_worker((worker), &__key);		\
> +	} while (0)
> +
> +#define init_kthread_work(work, fn)					\
> +	do {								\
> +		memset((work), 0, sizeof(struct kthread_work));		\
> +		INIT_LIST_HEAD(&(work)->node);				\
> +		(work)->func = (fn);					\
> +		init_waitqueue_head(&(work)->done);			\
> +		(work)->flushing = ATOMIC_INIT(0);			\
> +	} while (0)
> 
>  int kthread_worker_fn(void *worker_ptr);
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 2dc3786..fae2eff 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -265,6 +265,15 @@ int kthreadd(void *unused)
>  	return 0;
>  }
> 
> +void __init_kthread_worker(struct kthread_worker *worker,
> +				struct lock_class_key *key)
> +{
> +	spin_lock_init(&worker->lock);
> +	lockdep_set_class(&worker->lock, key);
> +	INIT_LIST_HEAD(&worker->work_list);
> +	worker->task == NULL;
                       ^^
                        |  
GCC should have griped, "Statement with no effect," or something
similar.  (Did it?)

Regards,
Andy

> +}
> +
>  /**
>   * kthread_worker_fn - kthread function to process kthread_worker
>   * @worker_ptr: pointer to initialized kthread_worker
> -- 
> 1.7.0.4

Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep

[Yong Zhang]

On Tue, Dec 21, 2010 at 1:16 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Mon, 2010-12-20 at 17:28 +0800, Yong Zhang wrote:
>> On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
>> > On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>> >> init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
>> >> initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
>> >> a dynamically allocated kthread_worker object's internal spinlock_t.
>> >> This causes lockdep to gripe:
>> >>
>> >>        INFO: trying to register non-static key.
>> >>        the code is fine but needs lockdep annotation.
>> >>        turning off the locking correctness validator.
>> >>
>> >> To keep lockdep happy, use spin_lock_init() for dynamically
>> >> allocated kthread_worker objects' internal spinlock_t.
>> >>
>> >> Reported-by: Nicolas <nicolas.mailhot@laposte.net>
>> >> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>> >>
>> >> Cc: Tejun Heo <tj@kernel.org>
>> >> Cc: Jarod Wilson <jarod@redhat.com>
>> >> Cc: Ingo Molnar <mingo@redhat.com>
>> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>
>> > This will make different kthead_worker->lock initialized with one same
>> > key.
>
> Well, that wouldn't be very useful. :P
>
>
>> > So we should put the real initializer to kernel/kthread.c
>> > and make init_kthread_worker() to be a MACRO.
>
> Sounds OK to me.  I'm not a lockdep expert and I made my initial patch
> with the sole intention of making this bugzilla report go away:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=662384
>
>
>> untested patch is here. Andy/Nicolas, is it ok for you?
>
> No, see my comments below.
>
>> ---
>> Subject: [PATCH] kthread_work: Make lockdep happy
>>
>> spinlock in kthread_worker and wait_queue_head in kthread_work
>> both should be lockdep annotated.
>> So change the interface to make it suiltable for CONFIG_LOCKDEP.
>>
>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>> ---
>> I'm not sure if it's possible to define a worker on stack?
>> So I left DEFINE_KTHREAD_WORKER() untouched.
>>
>>  include/linux/kthread.h |   37 +++++++++++++++++++++++++++----------
>>  kernel/kthread.c        |    9 +++++++++
>>  2 files changed, 36 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
>> index 685ea65..5d516b3 100644
>> --- a/include/linux/kthread.h
>> +++ b/include/linux/kthread.h
>> @@ -75,22 +75,39 @@ struct kthread_work {
>>       .flushing = ATOMIC_INIT(0),                                     \
>>       }
>>
>> +/* Is it possible to define a worker on stack? */
>
> This comment doesn't help a developer decide if this interface is OK to
> use.
>
> If there is an alternate preferred API for instantiating 1 (or more)
> thread(s) to handle work objects off of the stack, then the comment
> should point the reader to that API (e.g. singlethread_workqueue).
>
> To answer the question in the comment:
>
> It is possible to allocate a kthread worker off of the stack, but IMO it
> has little advantage over a singlethread_workqueue allocated off of the
> stack.
>
> ivtv only needed the kthread_worker API, because it has some deferred
> work with tight timing constraints.  ivtv sets the kthread_worker to
> SCHED_FIFO scheduling for ivtv work, which couldn't be done on a
> workqueue thread with the updated singlethread_workqueue implementation.
> Note that ivtv does *not* allocate its kthread worker off of the stack.

Just like Tejun has said, It is possible to allocate a kthread worker
on stack, so I can just delete that unhelpful comment and also touch
kthread_worker in V2.

>
>
>>  #define DEFINE_KTHREAD_WORKER(worker)                                        \
>>       struct kthread_worker worker = KTHREAD_WORKER_INIT(worker)
>>
>>  #define DEFINE_KTHREAD_WORK(work, fn)                                        \
>>       struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
>>
>> -static inline void init_kthread_worker(struct kthread_worker *worker)
>> -{
>> -     *worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
>> -}
>> -
>> -static inline void init_kthread_work(struct kthread_work *work,
>> -                                  kthread_work_func_t fn)
>> -{
>> -     *work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
>> -}
>> +#ifdef CONFIG_LOCKDEP
>> +# define KTHREAD_WORK_INIT_ONSTACK(work, fn)                         \
>> +     ({init_kthread_work((&work), fn); work})
>> +# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn)                               \
>> +     struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
>> +#else
>> +# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
>> +#endif
>> +
>> +extern void __init_kthread_worker(struct kthread_worker *worker,
>> +                                     struct lock_class_key *key);
>> +
>> +#define init_kthread_worker(worker)                                  \
>> +     do {                                                            \
>> +             static struct lock_class_key __key;                     \
>> +             __init_kthread_worker((worker), &__key);                \
>> +     } while (0)
>> +
>> +#define init_kthread_work(work, fn)                                  \
>> +     do {                                                            \
>> +             memset((work), 0, sizeof(struct kthread_work));         \
>> +             INIT_LIST_HEAD(&(work)->node);                          \
>> +             (work)->func = (fn);                                    \
>> +             init_waitqueue_head(&(work)->done);                     \
>> +             (work)->flushing = ATOMIC_INIT(0);                      \
>> +     } while (0)
>>
>>  int kthread_worker_fn(void *worker_ptr);
>>
>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>> index 2dc3786..fae2eff 100644
>> --- a/kernel/kthread.c
>> +++ b/kernel/kthread.c
>> @@ -265,6 +265,15 @@ int kthreadd(void *unused)
>>       return 0;
>>  }
>>
>> +void __init_kthread_worker(struct kthread_worker *worker,
>> +                             struct lock_class_key *key)
>> +{
>> +     spin_lock_init(&worker->lock);
>> +     lockdep_set_class(&worker->lock, key);
>> +     INIT_LIST_HEAD(&worker->work_list);
>> +     worker->task == NULL;
>                       ^^
>                        |
> GCC should have griped, "Statement with no effect," or something
> similar.  (Did it?)

Good catch.
Will change it.

Thanks,
Yong
-- 
Only stand for myself.