[PATCH 1/1] kthread: disable preemption during complete()

[PATCH 1/1] kthread: disable preemption during complete()

[Peter Boonstoppel]

After a kthread is created it signals the requester using complete()
and enters TASK_UNINTERRUPTIBLE. However, since complete() wakes up
the requesting thread this can cause a preemption. The preemption will
not remove the task from the runqueue (for that schedule() has to be
invoked directly).

This is a problem if directly after kthread creation you try to do a
kthread_bind(), which will block in HZ steps until the thread is off
the runqueue.

This patch disables preemption during complete(), since we call
schedule() directly afterwards, so it will correctly enter
TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
cpu hotplug significantly.

Change-Id: I856ddd4e01ebdb198ba90f343b4a0c5933fd2b23
Signed-off-by: Peter Boonstoppel <pboonstoppel@nvidia.com>
---
 kernel/kthread.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index b579af5..f681b14 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -16,6 +16,8 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/freezer.h>
+#include <linux/preempt.h>
+#include <linux/thread_info.h>
 #include <trace/events/sched.h>
 
 static DEFINE_SPINLOCK(kthread_create_lock);
@@ -113,7 +115,10 @@ static int kthread(void *_create)
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;
+	preempt_disable();
 	complete(&create->done);
+	clear_need_resched();
+	preempt_enable_no_resched();
 	schedule();
 
 	ret = -EINTR;
-- 
1.7.0.4

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Tejun Heo]

Hello,

On Tue, Jul 24, 2012 at 05:05:32PM -0700, Peter Boonstoppel wrote:
> After a kthread is created it signals the requester using complete()
> and enters TASK_UNINTERRUPTIBLE. However, since complete() wakes up
> the requesting thread this can cause a preemption. The preemption will
> not remove the task from the runqueue (for that schedule() has to be
> invoked directly).
> 
> This is a problem if directly after kthread creation you try to do a
> kthread_bind(), which will block in HZ steps until the thread is off
> the runqueue.
> 
> This patch disables preemption during complete(), since we call
> schedule() directly afterwards, so it will correctly enter
> TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
> cpu hotplug significantly.
> 
> Change-Id: I856ddd4e01ebdb198ba90f343b4a0c5933fd2b23

Is this from internal gerrit?  Can you please remove it before sending
things upstream?

>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/freezer.h>
> +#include <linux/preempt.h>
> +#include <linux/thread_info.h>
>  #include <trace/events/sched.h>
>  
>  static DEFINE_SPINLOCK(kthread_create_lock);
> @@ -113,7 +115,10 @@ static int kthread(void *_create)
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
>  	__set_current_state(TASK_UNINTERRUPTIBLE);
>  	create->result = current;
> +	preempt_disable();
>  	complete(&create->done);
> +	clear_need_resched();

Is the above really necessary given that you're calling
preempt_enable_no_resched() right after?

> +	preempt_enable_no_resched();
>  	schedule();

Some comments would be really nice.

Thanks.

-- 
tejun

RE: [PATCH 1/1] kthread: disable preemption during complete()

[Peter Boonstoppel]

After a kthread is created it signals the requester using complete()
and enters TASK_UNINTERRUPTIBLE. However, since complete() wakes up
the requesting thread this can cause a preemption. The preemption will
not remove the task from the runqueue (for that schedule() has to be
invoked directly).

This is a problem if directly after kthread creation you try to do a
kthread_bind(), which will block in HZ steps until the thread is off
the runqueue.

This patch disables preemption during complete(), since we call
schedule() directly afterwards, so it will correctly enter
TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
cpu hotplug significantly.

Signed-off-by: Peter Boonstoppel <pboonstoppel@nvidia.com>
---
 kernel/kthread.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index b579af5..757d8dd 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/freezer.h>
+#include <linux/preempt.h>
 #include <trace/events/sched.h>
 
 static DEFINE_SPINLOCK(kthread_create_lock);
@@ -113,7 +114,17 @@ static int kthread(void *_create)
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;
+
+	/*
+	 * Disable preemption so we enter TASK_UNINTERRUPTIBLE after
+	 * complete() instead of possibly being preempted. This speeds
+	 * up clients that do a kthread_bind() directly after
+	 * creation.
+	 */
+	preempt_disable();
 	complete(&create->done);
+	preempt_enable_no_resched();
+
 	schedule();
 
 	ret = -EINTR;
-- 
1.7.0.4

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Tejun Heo]

(cc'ing Oleg and Peter)

On Wed, Jul 25, 2012 at 03:35:32PM -0700, Peter Boonstoppel wrote:
> After a kthread is created it signals the requester using complete()
> and enters TASK_UNINTERRUPTIBLE. However, since complete() wakes up
> the requesting thread this can cause a preemption. The preemption will
> not remove the task from the runqueue (for that schedule() has to be
> invoked directly).
> 
> This is a problem if directly after kthread creation you try to do a
> kthread_bind(), which will block in HZ steps until the thread is off
> the runqueue.
> 
> This patch disables preemption during complete(), since we call
> schedule() directly afterwards, so it will correctly enter
> TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
> cpu hotplug significantly.
> 
> Signed-off-by: Peter Boonstoppel <pboonstoppel@nvidia.com>
> ---
>  kernel/kthread.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index b579af5..757d8dd 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -16,6 +16,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/freezer.h>
> +#include <linux/preempt.h>
>  #include <trace/events/sched.h>
>  
>  static DEFINE_SPINLOCK(kthread_create_lock);
> @@ -113,7 +114,17 @@ static int kthread(void *_create)
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
>  	__set_current_state(TASK_UNINTERRUPTIBLE);
>  	create->result = current;
> +
> +	/*
> +	 * Disable preemption so we enter TASK_UNINTERRUPTIBLE after
> +	 * complete() instead of possibly being preempted. This speeds
> +	 * up clients that do a kthread_bind() directly after
> +	 * creation.
> +	 */
> +	preempt_disable();

Shouldn't this happen before setting current state to UNINTERRUPTIBLE?
What prevents preemption happening right above preempt_disable()?

>  	complete(&create->done);
> +	preempt_enable_no_resched();
> +
>  	schedule();

PeterZ, Oleg, can you guys please review this?

Thanks.

-- 
tejun

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Peter Zijlstra]

On Wed, 2012-07-25 at 15:40 -0700, Tejun Heo wrote:
> (cc'ing Oleg and Peter)

Right, if you're playing games with preemption, always add the rt and
sched folks.. added mingo and tglx.

> On Wed, Jul 25, 2012 at 03:35:32PM -0700, Peter Boonstoppel wrote:
> > After a kthread is created it signals the requester using complete()
> > and enters TASK_UNINTERRUPTIBLE. However, since complete() wakes up
> > the requesting thread this can cause a preemption. The preemption will
> > not remove the task from the runqueue (for that schedule() has to be
> > invoked directly).
> > 
> > This is a problem if directly after kthread creation you try to do a
> > kthread_bind(), which will block in HZ steps until the thread is off
> > the runqueue.
> > 
> > This patch disables preemption during complete(), since we call
> > schedule() directly afterwards, so it will correctly enter
> > TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
> > cpu hotplug significantly.

tglx has patches that make the kthread create/destroy stuff from hotplug
go away.. that seems like the better approach.


> > Signed-off-by: Peter Boonstoppel <pboonstoppel@nvidia.com>
> > ---
> >  kernel/kthread.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index b579af5..757d8dd 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/freezer.h>
> > +#include <linux/preempt.h>
> >  #include <trace/events/sched.h>
> >  
> >  static DEFINE_SPINLOCK(kthread_create_lock);
> > @@ -113,7 +114,17 @@ static int kthread(void *_create)
> >  	/* OK, tell user we're spawned, wait for stop or wakeup */
> >  	__set_current_state(TASK_UNINTERRUPTIBLE);
> >  	create->result = current;
> > +
> > +	/*
> > +	 * Disable preemption so we enter TASK_UNINTERRUPTIBLE after
> > +	 * complete() instead of possibly being preempted. This speeds
> > +	 * up clients that do a kthread_bind() directly after
> > +	 * creation.
> > +	 */
> > +	preempt_disable();
> 
> Shouldn't this happen before setting current state to UNINTERRUPTIBLE?
> What prevents preemption happening right above preempt_disable()?

Nothing, it also doesn't matter that much, you could get preempted right
before preempt_disable() and end up in the same place.

The main thing is avoiding the wakeup preemption from the complete()
because we're going to sleep right after anyway.

The comment doesn't really make that clear.

> >  	complete(&create->done);
> > +	preempt_enable_no_resched();
> > +
> >  	schedule();

Other than that it seems fine, although I know tglx just loves new
preempt_enable_no_resched() sites ;-)

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Thomas Gleixner]

On Thu, 26 Jul 2012, Peter Zijlstra wrote:
> On Wed, 2012-07-25 at 15:40 -0700, Tejun Heo wrote:
> > > This patch disables preemption during complete(), since we call
> > > schedule() directly afterwards, so it will correctly enter
> > > TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
> > > cpu hotplug significantly.
> 
> tglx has patches that make the kthread create/destroy stuff from hotplug
> go away.. that seems like the better approach.

Right. That cpu hotplug setup/teardown stuff is ugly.

> > > +
> > > +	/*
> > > +	 * Disable preemption so we enter TASK_UNINTERRUPTIBLE after
> > > +	 * complete() instead of possibly being preempted. This speeds
> > > +	 * up clients that do a kthread_bind() directly after
> > > +	 * creation.
> > > +	 */
> > > +	preempt_disable();
> > 
> > Shouldn't this happen before setting current state to UNINTERRUPTIBLE?
> > What prevents preemption happening right above preempt_disable()?
> 
> Nothing, it also doesn't matter that much, you could get preempted right
> before preempt_disable() and end up in the same place.
> 
> The main thing is avoiding the wakeup preemption from the complete()
> because we're going to sleep right after anyway.
> 
> The comment doesn't really make that clear.

Right, the comment is crap. It has nothing to do with kthread_bind()
and stuff. The whole purpose is to avoid the pointless preemption
after wakeup.
 
> > >  	complete(&create->done);
> > > +	preempt_enable_no_resched();
> > > +
> > >  	schedule();
> 
> Other than that it seems fine, although I know tglx just loves new
> preempt_enable_no_resched() sites ;-)

The ones which are immediately followed by a call to schedule() are at
least not causing any headache for RT :)

Thanks,

	tglx

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Oleg Nesterov]

On 07/26, Thomas Gleixner wrote:
>
> On Thu, 26 Jul 2012, Peter Zijlstra wrote:
> > On Wed, 2012-07-25 at 15:40 -0700, Tejun Heo wrote:
> > > > This patch disables preemption during complete(), since we call
> > > > schedule() directly afterwards, so it will correctly enter
> > > > TASK_UNINTERRUPTIBLE. This speeds up kthread creation/binding during
> > > > cpu hotplug significantly.
> >
> > tglx has patches that make the kthread create/destroy stuff from hotplug
> > go away.. that seems like the better approach.
>
> Right. That cpu hotplug setup/teardown stuff is ugly.

Could you cc me if you send these patches?

> > The comment doesn't really make that clear.
>
> Right, the comment is crap. It has nothing to do with kthread_bind()
> and stuff. The whole purpose is to avoid the pointless preemption
> after wakeup.

Yes, but this "avoid the preemption after wakeup" can actually help
kthread_bind()->wait_task_inactive() ?

This reminds me, Peter had a patch which teaches wait_task_inactive()
to use sched_in/sched_out notifiers to avoid the polling...

Oleg.

Re: [PATCH 1/1] kthread: disable preemption during complete()

[Peter Zijlstra]

On Thu, 2012-07-26 at 17:54 +0200, Oleg Nesterov wrote:
> Yes, but this "avoid the preemption after wakeup" can actually help
> kthread_bind()->wait_task_inactive() ?

Yeah.

> This reminds me, Peter had a patch which teaches wait_task_inactive()
> to use sched_in/sched_out notifiers to avoid the polling... 

I did, but from what I could remember you shot holes in it and I didn't
find a way to plug them and not make it a bigger mess than it is now :/

RE: [PATCH 1/1] kthread: disable preemption during complete()

[Peter Boonstoppel]

> > tglx has patches that make the kthread create/destroy stuff from hotplug
> > go away.. that seems like the better approach.

> Right. That cpu hotplug setup/teardown stuff is ugly.

If that stuff gets removed complete that's great. The only change I'm aware of right now is the workqueue one: http://thread.gmane.org/gmane.linux.kernel/1329164

> > The main thing is avoiding the wakeup preemption from the complete()
> > because we're going to sleep right after anyway.

You are very likely to be preempted by the complete(), since the newly created thread has a relatively high vruntime.

> > The comment doesn't really make that clear.

> Right, the comment is crap. It has nothing to do with kthread_bind()
> and stuff. The whole purpose is to avoid the pointless preemption
> after wakeup.

The only case I want to solve is the kthread_bind()->wait_task_inactive() scenario. On our platforms this patch reduces average cpu_up() time from about 9ms to 8ms, but max time goes down from 37ms to 8.5ms. cpu_up() latency becomes much more predictable.


PeterB

RE: [PATCH 1/1] kthread: disable preemption during complete()

[Thomas Gleixner]

On Thu, 26 Jul 2012, Peter Boonstoppel wrote:

> > > tglx has patches that make the kthread create/destroy stuff from hotplug
> > > go away.. that seems like the better approach.
> 
> > Right. That cpu hotplug setup/teardown stuff is ugly.
> 
> If that stuff gets removed complete that's great. The only change I'm aware of right now is the workqueue one: http://thread.gmane.org/gmane.linux.kernel/1329164
> 
> > > The main thing is avoiding the wakeup preemption from the complete()
> > > because we're going to sleep right after anyway.
> 
> You are very likely to be preempted by the complete(), since the newly created thread has a relatively high vruntime.
> 
> > > The comment doesn't really make that clear.
> 
> > Right, the comment is crap. It has nothing to do with kthread_bind()
> > and stuff. The whole purpose is to avoid the pointless preemption
> > after wakeup.
> 
> The only case I want to solve is the kthread_bind()->wait_task_inactive() scenario. On our platforms this patch reduces average cpu_up() time from about 9ms to 8ms, but max time goes down from 37ms to 8.5ms. cpu_up() latency becomes much more predictable.
> 

There is a bunch of patches in the queue, which kills the full
setup/teardown of per cpu threads and puts those threads into a "park"
position instead.

Thanks,

	tglx