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

Groups > linux.kernel > #1641179 > unrolled thread

[patch 17/18] sched: Enable might_sleep() checks early

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [patch 17/18] sched: Enable might_sleep() checks early Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:40 +0200
    Re: [patch 17/18] sched: Enable might_sleep() checks early Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 17:20 +0200
      Re: [patch 17/18] sched: Enable might_sleep() checks early Thomas Gleixner <tglx@linutronix.de> - 2017-05-15 21:20 +0200
        Re: [patch 17/18] sched: Enable might_sleep() checks early Peter Zijlstra <peterz@infradead.org> - 2017-05-16 09:20 +0200
          Re: [patch 17/18] sched: Enable might_sleep() checks early Thomas Gleixner <tglx@linutronix.de> - 2017-05-16 09:40 +0200
            Re: [patch 17/18] sched: Enable might_sleep() checks early Steven Rostedt <rostedt@goodmis.org> - 2017-05-16 15:20 +0200

#1641179 — [patch 17/18] sched: Enable might_sleep() checks early

might_sleep() checks are enabled after the boot process is done. That hides
bugs in the smp bringup and driver initialization code.

Enable it right when the scheduler starts working, i.e. when init task and
kthreadd have been created and right before the idle task enables
preemption.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 init/main.c         |    2 ++
 kernel/sched/core.c |    4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

--- a/init/main.c
+++ b/init/main.c
@@ -410,6 +410,8 @@ static noinline void __ref rest_init(voi
 	 * at least once to get things moving:
 	 */
 	init_idle_bootup_task(current);
+	/* Enable might_sleep() checks */
+	system_state = SYSTEM_BOOTING_UP;
 	schedule_preempt_disabled();
 	/* Call into cpu_idle with preempt disabled */
 	cpu_startup_entry(CPUHP_ONLINE);
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6226,8 +6226,10 @@ void ___might_sleep(const char *file, in
 
 	if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
 	     !is_idle_task(current)) ||
-	    system_state != SYSTEM_RUNNING || oops_in_progress)
+	    system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
+	    oops_in_progress)
 		return;
+
 	if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
 		return;
 	prev_jiffy = jiffies;

[toc] | [next] | [standalone]

#1641792

On Sun, 14 May 2017 20:27:33 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> might_sleep() checks are enabled after the boot process is done. That hides
> bugs in the smp bringup and driver initialization code.
> 
> Enable it right when the scheduler starts working, i.e. when init task and
> kthreadd have been created and right before the idle task enables
> preemption.

Looking at commit b433c3d4549ae749, it appears that on very slow
machines, there is a possibility that the init task can start running.
Should system_state be updated before that complete() is called?

-- Steve

> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  init/main.c         |    2 ++
>  kernel/sched/core.c |    4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> --- a/init/main.c
> +++ b/init/main.c
> @@ -410,6 +410,8 @@ static noinline void __ref rest_init(voi
>  	 * at least once to get things moving:
>  	 */
>  	init_idle_bootup_task(current);
> +	/* Enable might_sleep() checks */
> +	system_state = SYSTEM_BOOTING_UP;
>  	schedule_preempt_disabled();
>  	/* Call into cpu_idle with preempt disabled */
>  	cpu_startup_entry(CPUHP_ONLINE);
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6226,8 +6226,10 @@ void ___might_sleep(const char *file, in
>  
>  	if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
>  	     !is_idle_task(current)) ||
> -	    system_state != SYSTEM_RUNNING || oops_in_progress)
> +	    system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
> +	    oops_in_progress)
>  		return;
> +
>  	if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
>  		return;
>  	prev_jiffy = jiffies;
> 

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

#1641985

On Mon, 15 May 2017, Steven Rostedt wrote:

> On Sun, 14 May 2017 20:27:33 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > might_sleep() checks are enabled after the boot process is done. That hides
> > bugs in the smp bringup and driver initialization code.
> > 
> > Enable it right when the scheduler starts working, i.e. when init task and
> > kthreadd have been created and right before the idle task enables
> > preemption.
> 
> Looking at commit b433c3d4549ae749, it appears that on very slow
> machines, there is a possibility that the init task can start running.
> Should system_state be updated before that complete() is called?

That commit is magic voodoo with exactly no effect at all.

rest_init() is called with preemption disabled and nothing can schedule
there _before_ schedule_preempt_disabled().

Both threads - init task and kthreadd - are only created and woken up. They
cannot get on the CPU simply because preemption is disabled. And this was
the case back then in 2.6.35 as well.

It does not matter at all whether the machine is slow or not. That
completion is pointless.

Peter, can you explain what the heck this patch is actually doing?

Thanks,

	tglx

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

#1642262

On Mon, May 15, 2017 at 09:12:03PM +0200, Thomas Gleixner wrote:
> On Mon, 15 May 2017, Steven Rostedt wrote:
> 
> > On Sun, 14 May 2017 20:27:33 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > might_sleep() checks are enabled after the boot process is done. That hides
> > > bugs in the smp bringup and driver initialization code.
> > > 
> > > Enable it right when the scheduler starts working, i.e. when init task and
> > > kthreadd have been created and right before the idle task enables
> > > preemption.
> > 
> > Looking at commit b433c3d4549ae749, it appears that on very slow
> > machines, there is a possibility that the init task can start running.
> > Should system_state be updated before that complete() is called?
> 
> That commit is magic voodoo with exactly no effect at all.
> 
> rest_init() is called with preemption disabled and nothing can schedule
> there _before_ schedule_preempt_disabled().
> 
> Both threads - init task and kthreadd - are only created and woken up. They
> cannot get on the CPU simply because preemption is disabled. And this was
> the case back then in 2.6.35 as well.
> 
> It does not matter at all whether the machine is slow or not. That
> completion is pointless.
> 
> Peter, can you explain what the heck this patch is actually doing?

Argh.. what a shit Changelog, who wrote that crap!?

So the problem was with PREEMPT_VOLUNTARY (where, as you know,
preempt_disable() has no meaning).

Supposedly there's a might_sleep()/cond_resched() point somewhere around
there (every alloc in the fork path for example), which will happily
reschedule us.

So if we schedule to the kernel_init() task before we set kthreadd_task
we'll try and spawn kthreads and OOPS.

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

#1642270

On Tue, 16 May 2017, Peter Zijlstra wrote:
> On Mon, May 15, 2017 at 09:12:03PM +0200, Thomas Gleixner wrote:
> > On Mon, 15 May 2017, Steven Rostedt wrote:
> > 
> > > On Sun, 14 May 2017 20:27:33 +0200
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > 
> > > > might_sleep() checks are enabled after the boot process is done. That hides
> > > > bugs in the smp bringup and driver initialization code.
> > > > 
> > > > Enable it right when the scheduler starts working, i.e. when init task and
> > > > kthreadd have been created and right before the idle task enables
> > > > preemption.
> > > 
> > > Looking at commit b433c3d4549ae749, it appears that on very slow
> > > machines, there is a possibility that the init task can start running.
> > > Should system_state be updated before that complete() is called?
> > 
> > That commit is magic voodoo with exactly no effect at all.
> > 
> > rest_init() is called with preemption disabled and nothing can schedule
> > there _before_ schedule_preempt_disabled().
> > 
> > Both threads - init task and kthreadd - are only created and woken up. They
> > cannot get on the CPU simply because preemption is disabled. And this was
> > the case back then in 2.6.35 as well.
> > 
> > It does not matter at all whether the machine is slow or not. That
> > completion is pointless.
> > 
> > Peter, can you explain what the heck this patch is actually doing?
> 
> Argh.. what a shit Changelog, who wrote that crap!?

Indeed.

> So the problem was with PREEMPT_VOLUNTARY (where, as you know,
> preempt_disable() has no meaning).
> 
> Supposedly there's a might_sleep()/cond_resched() point somewhere around
> there (every alloc in the fork path for example), which will happily
> reschedule us.

Darn, forgot about PREEMPT_VOLUNTARY and that excellent changelog does not
mention it either. 

> So if we schedule to the kernel_init() task before we set kthreadd_task
> we'll try and spawn kthreads and OOPS.

So back to Stevens question. No, we can't set the state earlier than right
before schedule() simply because with PREEMPT preemption _is_ actually
disabled and kernel_kthread() will trigger might_sleep() splats.

What a mess.

Thanks,

	tglx

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

#1642557

On Tue, 16 May 2017 09:33:52 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:


> Darn, forgot about PREEMPT_VOLUNTARY and that excellent changelog does not
> mention it either. 
> 
> > So if we schedule to the kernel_init() task before we set kthreadd_task
> > we'll try and spawn kthreads and OOPS.  
> 
> So back to Stevens question. No, we can't set the state earlier than right
> before schedule() simply because with PREEMPT preemption _is_ actually
> disabled and kernel_kthread() will trigger might_sleep() splats.

So a comment in the code mentioning PREEMPT_VOLUNTARY might be
advantageous.

> 
> What a mess.

Indeed.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web