Re: single-threaded wq lockdep is broken

From: Lai Jiangshan
Date: Wed May 31 2017 - 04:34:12 EST


On Mon, May 29, 2017 at 3:33 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> Hi Tejun,
>
> I suspect this is a long-standing bug introduced by all the pool rework
> you did at some point, but I don't really know nor can I figure out how
> to fix it right now. I guess it could possibly also be a lockdep issue,
> or an issue in how it's used, but I definitely know that this used to
> work (i.e. warn) back when I introduced the lockdep checking to the WQ
> code. I was actually bitten by a bug like this, and erroneously
> dismissed it as not being the case because lockdep hadn't warned (and
> the actual deadlock debug output is basically not existent).
>
> In any case, the following code really should result in a warning from
> lockdep, but doesn't. If you comment in the #define DEADLOCK, it will
> actually cause a deadlock :)
>
>
> #include <linux/kernel.h>
> #include <linux/mutex.h>
> #include <linux/workqueue.h>
> #include <linux/module.h>
> #include <linux/delay.h>
>
> DEFINE_MUTEX(mtx);
> static struct workqueue_struct *wq;
> static struct work_struct w1, w2;
>
> static void w1_wk(struct work_struct *w)
> {
> mutex_lock(&mtx);
> msleep(100);
> mutex_unlock(&mtx);
> }
>
> static void w2_wk(struct work_struct *w)
> {
> }
>
> /*
> * if not defined, then lockdep should warn only,

I guess when DEADLOCK not defined, there is no
work is queued nor executed, therefore, no lock
dependence is recorded, and there is no warn
either.

> * if defined, the system will really deadlock.
> */
>
> //#define DEADLOCK
>
> static int init(void)
> {
> wq = create_singlethread_workqueue("test");
> if (!wq)
> return -ENOMEM;
> INIT_WORK(&w1, w1_wk);
> INIT_WORK(&w2, w2_wk);
>

/* add lock dependence, the lockdep should warn */
queue_work(wq, &w1);
queue_work(wq, &w2);
flush_work(&w1);

> #ifdef DEADLOCK
> queue_work(wq, &w1);
> queue_work(wq, &w2);
> #endif
> mutex_lock(&mtx);
> flush_work(&w2);
> mutex_unlock(&mtx);
>
> #ifndef DEADLOCK
> queue_work(wq, &w1);
> queue_work(wq, &w2);
> #endif
>
> return 0;
> }
> module_init(init);
>
>
> (to test, just copy it to some C file and add "obj-y += myfile.o" to
> the Makefile in that directory, then boot the kernel - perhaps in a VM)
>
> johannes