Re: single-threaded wq lockdep is broken

[Lai Jiangshan]

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