single-threaded wq lockdep is broken

DISCUSSION Johannes Berg 3 participants 9 messages May 28, 2017 – Jun 13, 2017

Share & tools

Johannes Berg

author

2017-05-28 19:33 UTC #0 lore raw

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,
* 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);

#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

Lai Jiangshan

2017-05-31 08:34 UTC #1 lore raw

Quoted 33 lines · Johannes Berg

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);
}

Quoted 6 lines

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.

Quoted 5 lines

* if defined, the system will really deadlock.
*/
//#define DEADLOCK

Quoted 8 lines

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);

Quoted 22 lines

#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

Johannes Berg

author

2017-05-31 08:36 UTC #2 lore raw

Hi,

Quoted 43 lines

Quoted 17 lines
#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);
}
Quoted 6 lines
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.
Quoted 5 lines
* if defined, the system will really deadlock.
*/
//#define DEADLOCK
Quoted 8 lines
static int init(void)
{
        wq = create_singlethread_workqueue("test"); if (!wq)
                return -ENOMEM; INIT_WORK(&w1, w1_wk); INIT_WORK(&w2, w2_wk);

Quoted 17 lines

        /* add lock dependence, the lockdep should warn */ queue_work(wq, &w1);
        queue_work(wq, &w2);
        flush_work(&w1);
Quoted 12 lines
#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

This was "ifndef", so it does in fact run here, just like you suggested. It doesn't warn though.

I don't think the order of queue/flush would matter, in fact, if you insert it like you did, with the flush outside the mutex, no issue exists (until the later flush)

johannes

Johannes Berg

author

2017-05-31 08:47 UTC #3 lore raw

Quoted 3 lines · Johannes Berg

This was "ifndef", so it does in fact run here, just like you suggested. It doesn't warn though.

Also, even if DEADLOCK *is* defined, lockdep doesn't report anything.

johannes

Tejun Heo

2017-05-31 18:20 UTC #6 lore raw

In reply to Johannes Berg

Hello, Johannes.

Quoted 5 lines · Johannes Berg

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

Hah, didn't know this worked.

Quoted 7 lines

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 :)

...

Quoted 6 lines

static void w1_wk(struct work_struct *w)
{
	mutex_lock(&mtx);
	msleep(100);
	mutex_unlock(&mtx);
}

...

Quoted 8 lines

static int init(void)

{
	wq = create_singlethread_workqueue("test");
	if (!wq)
		return -ENOMEM;
	INIT_WORK(&w1, w1_wk);
	INIT_WORK(&w2, w2_wk);

Quoted 7 lines

#ifdef DEADLOCK
	queue_work(wq, &w1);
	queue_work(wq, &w2);
#endif
	mutex_lock(&mtx);
	flush_work(&w2);
	mutex_unlock(&mtx);

So, it used to always create dependency between work items on singlethread workqueues according to their queeing order? It shouldn't be difficult to fix. I'll dig through the history and see what happened.

Thanks.

Signature · 1 line

tejun

Johannes Berg

author

2017-05-31 19:16 UTC #7 lore raw

Hi Tejun,

Quoted 12 lines · Johannes Berg

Quoted 10 lines
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

Quoted 1 line

Hah, didn't know this worked.

Ah, it was nice when I made this work - but you won't believe the number of times I had to answer the question "what does this mean?" :-)

Quoted 4 lines

So, it used to always create dependency between work items on singlethread workqueues according to their queeing order? It shouldn't be difficult to fix. I'll dig through the history and see what happened.

No, queuing order is (was) irrelevant, and I'm not sure it should really matter all that much, since you often can't really predict queueing order. It used to be that this triggered a lot on the system_wq, which of course is no longer single-threaded so I suppose it can make progress even in situations like this?

It basically just did a dependency of wq->work, work->mutex (according to my code) and mutex->wq due to the flush.

I think that perhaps the last dependency of mutex->wq is lost now due to flush_work()? Or perhaps there's something with the read/write thing that caused this issue.

johannes

Lai Jiangshan

2017-06-02 07:04 UTC #4 lore raw

In reply to Johannes Berg

Quoted 63 lines · Johannes Berg

Hi,
Quoted 43 lines
Quoted 17 lines
#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);
}
Quoted 6 lines
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.
Quoted 5 lines
* if defined, the system will really deadlock.
*/
//#define DEADLOCK
Quoted 8 lines
static int init(void)
{
        wq = create_singlethread_workqueue("test");
        if (!wq)
                return -ENOMEM;
        INIT_WORK(&w1, w1_wk);
        INIT_WORK(&w2, w2_wk);
Quoted 17 lines
        /* add lock dependence, the lockdep should warn */
        queue_work(wq, &w1);
        queue_work(wq, &w2);
        flush_work(&w1);
Quoted 12 lines
#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

Quoted 7 lines

This was "ifndef", so it does in fact run here, just like you suggested. It doesn't warn though.
I don't think the order of queue/flush would matter, in fact, if you insert it like you did, with the flush outside the mutex, no issue exists (until the later flush)

the @w2 is not queued before flush_work(&w2), it is expected that @w2 is not associated with @wq, and the dependence mtx -> wq will not be recorded. And it is expected no warning.

Quoted 1 line

Also, even if DEADLOCK *is* defined, lockdep doesn't report anything.

Uhhh..... I have no idea about it yet.

Johannes Berg

author

2017-06-02 08:51 UTC #5 lore raw

Quoted 4 lines · Lai Jiangshan

the @w2 is not queued before flush_work(&w2), it is expected that @w2 is not associated with @wq, and the dependence mtx -> wq will not be recorded. And it is expected no warning.

Lockdep is symmetric. So then maybe it won't warn when executing flush_work(), but should later when executing @w2. No real difference?

johannes

Tejun Heo

2017-06-13 16:06 UTC #8 lore raw

In reply to Johannes Berg

Hello,

Johannes reported that lockdep warning on single threaded workqueues doesn't work anymore. Nothing really changed there and all the relevant lockdep annotaitons are being invoked correctly. The following is the extracted lockdep-only reproducer.

The culprit seems to be lock_map_acquire_read() being mapped to lock_map_acquire_shared_recursive() instead of lock_map_acquire_shared(). Is the following expected to not trigger lockdep warning? Should workqueue be using rwsem_acquire[_read]/release() instead of lock_map*()?

Thanks.

#include <linux/kernel.h>
#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/module.h>

DEFINE_MUTEX(mtx);

static int init(void)

{
	static struct lock_class_key key;
	struct lockdep_map *map;

	printk("XXX lockdep test start\n");

	map = kzalloc(sizeof(*map), GFP_KERNEL);
	lockdep_init_map(map, "test_map", &key, 0);

	mutex_lock(&mtx);
	lock_map_acquire(map);
	lock_map_release(map);
	mutex_unlock(&mtx);

	lock_map_acquire_read(map);
	mutex_lock(&mtx);
	mutex_unlock(&mtx);
	lock_map_release(map);

	printk("XXX lockdep test end\n");
	return 0;
}

module_init(init);