Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlockproperly for lockdep

[Yong Zhang]

On Tue, Dec 21, 2010 at 1:16 AM, Andy Walls <awalls@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
>> >> Signed-off-by: Andy Walls <awalls@xxxxxxxxxxxxxxxx>
>> >>
>> >> Cc: Tejun Heo <tj@xxxxxxxxxx>
>> >> Cc: Jarod Wilson <jarod@xxxxxxxxxx>
>> >> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> >> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>> >> Cc: Hans Verkuil <hverkuil@xxxxxxxxx>
>
>> > 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@xxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/