lkml.org 
[lkml]   [2010]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] kthread_worker: Initialize dynamically allocated spinlock properly for lockdep
    From
    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2010-12-20 10:31    [W:3.311 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site