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 <[email protected]>
Signed-off-by: Andy Walls <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Jarod Wilson <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Hans Verkuil <[email protected]>
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,
On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <[email protected]> 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 <[email protected]>
> Signed-off-by: Andy Walls <[email protected]>
>
> Cc: Tejun Heo <[email protected]>
> Cc: Jarod Wilson <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Hans Verkuil <[email protected]>
>
> 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.
On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <[email protected]> wrote:
> On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <[email protected]> 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 <[email protected]>
>> Signed-off-by: Andy Walls <[email protected]>
>>
>> Cc: Tejun Heo <[email protected]>
>> Cc: Jarod Wilson <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: Hans Verkuil <[email protected]>
>>
>> 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 <[email protected]>
---
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.
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 <[email protected]>
> ---
> 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
On Mon, 2010-12-20 at 17:28 +0800, Yong Zhang wrote:
> On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <[email protected]> wrote:
> > On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Andy Walls <[email protected]>
> >>
> >> Cc: Tejun Heo <[email protected]>
> >> Cc: Jarod Wilson <[email protected]>
> >> Cc: Ingo Molnar <[email protected]>
> >> Cc: Mauro Carvalho Chehab <[email protected]>
> >> Cc: Hans Verkuil <[email protected]>
> > 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 <[email protected]>
> ---
> 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
On Tue, Dec 21, 2010 at 12:21 AM, Tejun Heo <[email protected]> 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 <[email protected]>
>> ---
>> 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.
On Tue, Dec 21, 2010 at 1:16 AM, Andy Walls <[email protected]> wrote:
> On Mon, 2010-12-20 at 17:28 +0800, Yong Zhang wrote:
>> On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <[email protected]> wrote:
>> > On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <[email protected]> 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 <[email protected]>
>> >> Signed-off-by: Andy Walls <[email protected]>
>> >>
>> >> Cc: Tejun Heo <[email protected]>
>> >> Cc: Jarod Wilson <[email protected]>
>> >> Cc: Ingo Molnar <[email protected]>
>> >> Cc: Mauro Carvalho Chehab <[email protected]>
>> >> Cc: Hans Verkuil <[email protected]>
>
>> > 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 <[email protected]>
>> ---
>> 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.
From: Yong Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Yong Zhang <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Walls <[email protected]>
---
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
Hello,
On Tue, Dec 21, 2010 at 12:40:50PM +0800, Yong Zhang wrote:
> From: Yong Zhang <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Yong Zhang <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Walls <[email protected]>
> ---
> 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
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
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 <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Yong Zhang <[email protected]>
> > Cc: Tejun Heo <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Andy Walls <[email protected]>
> > ---
> > 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
On Tue, 2010-12-21 at 12:40 +0800, Yong Zhang wrote:
> From: Yong Zhang <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Yong Zhang <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Walls <[email protected]>
> ---
> 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 <[email protected]>
Signed-off-by: Andy Walls <[email protected]>
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
On Wed, Dec 22, 2010 at 8:39 AM, Andy Walls <[email protected]> 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 <[email protected]>
> Signed-off-by: Andy Walls <[email protected]>
>
> 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.
From: Yong Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Yong Zhang <[email protected]>
Signed-off-by: Andy Walls <[email protected]>
Tested-by: Andy Walls <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
On Wed, Dec 22, 2010 at 11:23:05AM +0800, Yong Zhang wrote:
> From: Yong Zhang <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Yong Zhang <[email protected]>
> Signed-off-by: Andy Walls <[email protected]>
> Tested-by: Andy Walls <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Andrew Morton <[email protected]>
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