[PATCH] slow_work_thread() should do the exclusive wait

[PATCH] slow_work_thread() should do the exclusive wait

[David Howells]

From: Oleg Nesterov <oleg@redhat.com>

slow_work_thread() sleeps on slow_work_thread_wq without WQ_FLAG_EXCLUSIVE,
this means that slow_work_enqueue()->__wake_up(nr_exclusive => 1) wakes up all
kslowd threads.  This is not what we want, so we change slow_work_thread() to
use prepare_to_wait_exclusive() instead.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 kernel/slow-work.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/kernel/slow-work.c b/kernel/slow-work.c
index b28d191..521ed20 100644
--- a/kernel/slow-work.c
+++ b/kernel/slow-work.c
@@ -372,8 +372,8 @@ static int slow_work_thread(void *_data)
 		vsmax *= atomic_read(&slow_work_thread_count);
 		vsmax /= 100;
 
-		prepare_to_wait(&slow_work_thread_wq, &wait,
-				TASK_INTERRUPTIBLE);
+		prepare_to_wait_exclusive(&slow_work_thread_wq, &wait,
+					  TASK_INTERRUPTIBLE);
 		if (!freezing(current) &&
 		    !slow_work_threads_should_exit &&
 		    !slow_work_available(vsmax) &&

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

(add Ingo)

On 04/23, David Howells wrote:
>
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > I wonder if slow_work_cull_timeout() should have some sort of barrier,
> > so the write is suitably visible to the woken thread.  Bearing in mind
> > that the thread might _already_ have been woken by someone else?
>
> Perhaps the attached patch?
>
> David
> ---
> From: David Howells <dhowells@redhat.com>
> Subject: [PATCH] slow_work_cull_timeout() should have a memory barrier
>
> slow_work_cull_timeout() should have a write memory barrier so that the setting
> of the cull flag is seen before the wakeup takes place.  This is required
> because wake_up() does not guarantee any memory barriership at all.
>
> Concomitant to this, slow_work_thread() should have a read memory barrier
> between its return from schedule() and its testing of slow_work_cull() as
> finish_wait() isn't a guaranteed barrier either.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  kernel/slow-work.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
>
> diff --git a/kernel/slow-work.c b/kernel/slow-work.c
> index 521ed20..96e418d 100644
> --- a/kernel/slow-work.c
> +++ b/kernel/slow-work.c
> @@ -382,6 +382,7 @@ static int slow_work_thread(void *_data)
>  		finish_wait(&slow_work_thread_wq, &wait);
>
>  		try_to_freeze();
> +		smp_rmb();
>
>  		vsmax = vslow_work_proportion;
>  		vsmax *= atomic_read(&slow_work_thread_count);
> @@ -416,6 +417,7 @@ static int slow_work_thread(void *_data)
>  static void slow_work_cull_timeout(unsigned long data)
>  {
>  	slow_work_cull = true;
> +	smp_wmb();
>  	wake_up(&slow_work_thread_wq);
>  }

Confused. If we need this barrier, a lot of similar code is broken.

	slow_work_cull_timeout:

		slow_work_cull = true;
		wake_up(&slow_work_thread_wq);


	slow_work_thread:

		prepare_to_wait(&slow_work_thread_wq);
		if (!slow_work_cull)
			schedule();
		finish_wait(&slow_work_thread_wq);

		if (slow_work_cull)
			.....

Both wake_up() and prepare_to_wait() take the same wait_queue_head_t->lock,
and prepare_to_wait() does set_current_state() under this lock.

How can we miss the event? If wake_up() happens before prepare_to_wait(),
slow_work_thread() must see slow_work_cull = T, otherwise the subsequent
wake_up() must see the result of list_add() + set_current_state() and
wake up the sleeping thread.

Could you please clarify?

Oleg.

Re: [PATCH] slow_work_thread() should do the exclusive wait

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> I wonder if slow_work_cull_timeout() should have some sort of barrier,
> so the write is suitably visible to the woken thread.  Bearing in mind
> that the thread might _already_ have been woken by someone else?

Perhaps the attached patch?

David
---
From: David Howells <dhowells@redhat.com>
Subject: [PATCH] slow_work_cull_timeout() should have a memory barrier

slow_work_cull_timeout() should have a write memory barrier so that the setting
of the cull flag is seen before the wakeup takes place.  This is required
because wake_up() does not guarantee any memory barriership at all.

Concomitant to this, slow_work_thread() should have a read memory barrier
between its return from schedule() and its testing of slow_work_cull() as
finish_wait() isn't a guaranteed barrier either.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 kernel/slow-work.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


diff --git a/kernel/slow-work.c b/kernel/slow-work.c
index 521ed20..96e418d 100644
--- a/kernel/slow-work.c
+++ b/kernel/slow-work.c
@@ -382,6 +382,7 @@ static int slow_work_thread(void *_data)
 		finish_wait(&slow_work_thread_wq, &wait);
 
 		try_to_freeze();
+		smp_rmb();
 
 		vsmax = vslow_work_proportion;
 		vsmax *= atomic_read(&slow_work_thread_count);
@@ -416,6 +417,7 @@ static int slow_work_thread(void *_data)
 static void slow_work_cull_timeout(unsigned long data)
 {
 	slow_work_cull = true;
+	smp_wmb();
 	wake_up(&slow_work_thread_wq);
 }
 

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

On 04/16, David Howells wrote:
>
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > I wonder if slow_work_cull_timeout() should have some sort of barrier,
> > so the write is suitably visible to the woken thread.
>
> That's an interesting question.  Should wake_up() imply a barrier of any sort,
> I wonder.  Well, __wake_up() does impose a barrier as it uses a spinlock, but
> I wonder if that's sufficient.

wake_up() does imply the barrier. Note the smp_wmb() in try_to_wake_up().
And in fact this wmb() implies mb(), because spin_lock() itself is STORE,
and the futher LOADs can't leak up before spin_lock().

But afaics, this doesn't matter? prepare_to_wait() sets task->state under
wait_queue_head_t->lock and wake_up() takes this look too, so we can't miss
the event.

Or I completely misunderstood the issue...

Oleg.

Re: [PATCH] slow_work_thread() should do the exclusive wait

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> The patch itself is a little worrisome.  The wake-all semantics are
> very good at covering up little race bugs.  And switching to wake-once
> is a great way of exposing hitherto-unsuspected races.

It's something I'm intending to test, once I get MN10300 working again (which
for some reason it isn't).

> I wonder if slow_work_cull_timeout() should have some sort of barrier,
> so the write is suitably visible to the woken thread.

That's an interesting question.  Should wake_up() imply a barrier of any sort,
I wonder.  Well, __wake_up() does impose a barrier as it uses a spinlock, but
I wonder if that's sufficient.

> Bearing in mind that the thread might _already_ have been woken by someone
> else?

If the thread is woken by someone else, there must be work for it to do, in
which case it wouldn't be culled anyway.

> off-topic: afacit the code will cull a maximum of one thread per five
> seconds.  But the rate of thread _creation_ is, afacit, unbound.  Are
> there scenarios in which we can get a runaway thread count?

The maximum number of threads is limited (slow_work_max_threads).

David

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Andrew Morton]

On Tue, 14 Apr 2009 00:24:51 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/13, Trond Myklebust wrote:
> >
> > On Mon, 2009-04-13 at 23:48 +0200, Oleg Nesterov wrote:
> > > On 04/13, David Howells wrote:
> > > >
> > > > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > > >
> > > > > Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> > > > > in the enclosing for(;;) loop that checks for or handles signals...
> > > >
> > > > If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not
> > > > doing anything.  I must admit, I thought I was calling daemonize(), but that
> > > > seems to have got lost somewhere.
> > >
> > > daemonize() is not needed, kthread_create() creates the kernel thread which
> > > ignores all signals. So it doesn't matter which state we use to sleep,
> > > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE.
> >
> > Yes, but that is precisely why it is cleaner to use
> > TASK_UNINTERRUPTIBLE. It documents the fact that signal handling isn't
> > needed (whether or not the thread is blocking them).
> 
> Agreed. But TASK_UNINTERRUPTIBLE can confuse a user which does
> "cat /proc/loadavg" on the idle machine...
> 
> Note that, for example, worker_thread() uses TASK_INTERRUPTIBLE too, and I
> think for the same reason.
> 

Yup.  It's a very common pattern for kernel threads to sleep in state
TASK_INTERRUPTIBLE.  It is "well known" (lol) that kernel threads don't
accept signals, and having a kernel thread sleep in state
TASK_UNINTERRUPTIBLE will indeed contribute to load average and we get
distressed emails quite promptly when we do that.

The patch itself is a little worrisome.  The wake-all semantics are
very good at covering up little race bugs.  And switching to wake-once
is a great way of exposing hitherto-unsuspected races.

<looks for races>

Nothing immediately leaps out, but you know how these things are.

I wonder if slow_work_cull_timeout() should have some sort of barrier,
so the write is suitably visible to the woken thread.  Bearing in mind
that the thread might _already_ have been woken by someone else?


off-topic: afacit the code will cull a maximum of one thread per five
seconds.  But the rate of thread _creation_ is, afacit, unbound.  Are
there scenarios in which we can get a runaway thread count?

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

On 04/13, Trond Myklebust wrote:
>
> On Mon, 2009-04-13 at 23:48 +0200, Oleg Nesterov wrote:
> > On 04/13, David Howells wrote:
> > >
> > > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > >
> > > > Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> > > > in the enclosing for(;;) loop that checks for or handles signals...
> > >
> > > If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not
> > > doing anything.  I must admit, I thought I was calling daemonize(), but that
> > > seems to have got lost somewhere.
> >
> > daemonize() is not needed, kthread_create() creates the kernel thread which
> > ignores all signals. So it doesn't matter which state we use to sleep,
> > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE.
>
> Yes, but that is precisely why it is cleaner to use
> TASK_UNINTERRUPTIBLE. It documents the fact that signal handling isn't
> needed (whether or not the thread is blocking them).

Agreed. But TASK_UNINTERRUPTIBLE can confuse a user which does
"cat /proc/loadavg" on the idle machine...

Note that, for example, worker_thread() uses TASK_INTERRUPTIBLE too, and I
think for the same reason.

I dunno.

Oleg.

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Trond Myklebust]

On Mon, 2009-04-13 at 23:48 +0200, Oleg Nesterov wrote:
> On 04/13, David Howells wrote:
> >
> > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> >
> > > Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> > > in the enclosing for(;;) loop that checks for or handles signals...
> >
> > If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not
> > doing anything.  I must admit, I thought I was calling daemonize(), but that
> > seems to have got lost somewhere.
> 
> daemonize() is not needed, kthread_create() creates the kernel thread which
> ignores all signals. So it doesn't matter which state we use to sleep,
> TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE.

Yes, but that is precisely why it is cleaner to use
TASK_UNINTERRUPTIBLE. It documents the fact that signal handling isn't
needed (whether or not the thread is blocking them).

Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

On 04/13, David Howells wrote:
>
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
>
> > Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> > in the enclosing for(;;) loop that checks for or handles signals...
>
> If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not
> doing anything.  I must admit, I thought I was calling daemonize(), but that
> seems to have got lost somewhere.

daemonize() is not needed, kthread_create() creates the kernel thread which
ignores all signals. So it doesn't matter which state we use to sleep,
TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE.

Oleg.

Re: [PATCH] slow_work_thread() should do the exclusive wait

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> in the enclosing for(;;) loop that checks for or handles signals...

If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not
doing anything.  I must admit, I thought I was calling daemonize(), but that
seems to have got lost somewhere.

David

Re: [PATCH] slow_work_thread() should do the exclusive wait

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> slow_work_thread() sleeps on slow_work_thread_wq without WQ_FLAG_EXCLUSIVE,
> this means that slow_work_enqueue()->__wake_up(nr_exclusive => 1) wakes up
> all kslowd threads. Afaics this is not what we want, change slow_work_thread()
> to use prepare_to_wait_exclusive().

Hmmm...  I think you may be right.  I think I was assuming that wake_up()
would only wake up the first item on the queue, but that's not strictly what
it does...

David

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

On 04/13, Trond Myklebust wrote:
>
> On Mon, 2009-04-13 at 20:17 +0200, Oleg Nesterov wrote:
> > slow_work_thread() sleeps on slow_work_thread_wq without WQ_FLAG_EXCLUSIVE,
> > this means that slow_work_enqueue()->__wake_up(nr_exclusive => 1) wakes up
> > all kslowd threads. Afaics this is not what we want, change slow_work_thread()
> > to use prepare_to_wait_exclusive().
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > 
> > --- 6.30/kernel/slow-work.c~1_SW_EXCLUSIVE	2009-04-06 00:03:42.000000000 +0200
> > +++ 6.30/kernel/slow-work.c	2009-04-13 19:40:20.000000000 +0200
> > @@ -372,8 +372,8 @@ static int slow_work_thread(void *_data)
> >  		vsmax *= atomic_read(&slow_work_thread_count);
> >  		vsmax /= 100;
> >  
> > -		prepare_to_wait(&slow_work_thread_wq, &wait,
> > -				TASK_INTERRUPTIBLE);
> > +		prepare_to_wait_exclusive(&slow_work_thread_wq, &wait,
> > +						TASK_INTERRUPTIBLE);
> >  		if (!freezing(current) &&
> >  		    !slow_work_threads_should_exit &&
> >  		    !slow_work_available(vsmax) &&
> > 
> 
> Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
> in the enclosing for(;;) loop that checks for or handles signals...

I guess TASK_INTERRUPTIBLE was chosen to not contribute to calc_load(),
nr_active() returns nr_running + nr_uninterruptible.

Oleg.

Re: [PATCH] slow_work_thread() should do the exclusive wait

[Trond Myklebust]

On Mon, 2009-04-13 at 20:17 +0200, Oleg Nesterov wrote:
> slow_work_thread() sleeps on slow_work_thread_wq without WQ_FLAG_EXCLUSIVE,
> this means that slow_work_enqueue()->__wake_up(nr_exclusive => 1) wakes up
> all kslowd threads. Afaics this is not what we want, change slow_work_thread()
> to use prepare_to_wait_exclusive().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> --- 6.30/kernel/slow-work.c~1_SW_EXCLUSIVE	2009-04-06 00:03:42.000000000 +0200
> +++ 6.30/kernel/slow-work.c	2009-04-13 19:40:20.000000000 +0200
> @@ -372,8 +372,8 @@ static int slow_work_thread(void *_data)
>  		vsmax *= atomic_read(&slow_work_thread_count);
>  		vsmax /= 100;
>  
> -		prepare_to_wait(&slow_work_thread_wq, &wait,
> -				TASK_INTERRUPTIBLE);
> +		prepare_to_wait_exclusive(&slow_work_thread_wq, &wait,
> +						TASK_INTERRUPTIBLE);
>  		if (!freezing(current) &&
>  		    !slow_work_threads_should_exit &&
>  		    !slow_work_available(vsmax) &&
> 

Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious
in the enclosing for(;;) loop that checks for or handles signals...

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[PATCH] slow_work_thread() should do the exclusive wait

[Oleg Nesterov]

slow_work_thread() sleeps on slow_work_thread_wq without WQ_FLAG_EXCLUSIVE,
this means that slow_work_enqueue()->__wake_up(nr_exclusive => 1) wakes up
all kslowd threads. Afaics this is not what we want, change slow_work_thread()
to use prepare_to_wait_exclusive().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.30/kernel/slow-work.c~1_SW_EXCLUSIVE	2009-04-06 00:03:42.000000000 +0200
+++ 6.30/kernel/slow-work.c	2009-04-13 19:40:20.000000000 +0200
@@ -372,8 +372,8 @@ static int slow_work_thread(void *_data)
 		vsmax *= atomic_read(&slow_work_thread_count);
 		vsmax /= 100;
 
-		prepare_to_wait(&slow_work_thread_wq, &wait,
-				TASK_INTERRUPTIBLE);
+		prepare_to_wait_exclusive(&slow_work_thread_wq, &wait,
+						TASK_INTERRUPTIBLE);
 		if (!freezing(current) &&
 		    !slow_work_threads_should_exit &&
 		    !slow_work_available(vsmax) &&