Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652199 > unrolled thread

single-threaded wq lockdep is broken

Back to article view | Back to linux.kernel

Contents

  single-threaded wq lockdep is broken Johannes Berg <johannes@sipsolutions.net> - 2017-05-28 21:40 +0200
    Re: single-threaded wq lockdep is broken Lai Jiangshan <jiangshanlai@gmail.com> - 2017-05-31 10:40 +0200
      Re: single-threaded wq lockdep is broken Johannes Berg <johannes@sipsolutions.net> - 2017-05-31 10:40 +0200
        Re: single-threaded wq lockdep is broken Johannes Berg <johannes@sipsolutions.net> - 2017-05-31 10:50 +0200
        Re: single-threaded wq lockdep is broken Lai Jiangshan <jiangshanlai@gmail.com> - 2017-06-02 09:10 +0200
          Re: single-threaded wq lockdep is broken Johannes Berg <johannes@sipsolutions.net> - 2017-06-02 11:00 +0200
    Re: single-threaded wq lockdep is broken Tejun Heo <tj@kernel.org> - 2017-05-31 20:30 +0200
      Re: single-threaded wq lockdep is broken Johannes Berg <johannes@sipsolutions.net> - 2017-05-31 21:20 +0200

#1652199 — single-threaded wq lockdep is broken

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

[toc] | [next] | [standalone]

#1654009

On Mon, May 29, 2017 at 3:33 AM, Johannes Berg
<johannes@sipsolutions.net> 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

[toc] | [prev] | [next] | [standalone]

#1654010

Hi,

> > #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

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

[toc] | [prev] | [next] | [standalone]

#1654014

On Wed, 2017-05-31 at 10:36 +0200, Johannes Berg wrote:
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1655943

On Wed, May 31, 2017 at 4:36 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Hi,
>
>> > #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
>
> 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.

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

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

[toc] | [prev] | [next] | [standalone]

#1656027

On Fri, 2017-06-02 at 15:03 +0800, Lai Jiangshan wrote:
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1654533

Hello, Johannes.

On Sun, May 28, 2017 at 09:33:13PM +0200, Johannes Berg wrote:
> 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.

> 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 :)
...
> static void w1_wk(struct work_struct *w)
> {
> 	mutex_lock(&mtx);
> 	msleep(100);
> 	mutex_unlock(&mtx);
> }
...
> 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);

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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1654559

Hi Tejun,

> On Sun, May 28, 2017 at 09:33:13PM +0200, Johannes Berg wrote:
> > 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.

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

> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web