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

Groups > linux.kernel > #1641187 > unrolled thread

[patch 00/18] init: Enable might_sleep() and smp_processor_id() debugging early

Back to article view | Back to linux.kernel

Contents

  [patch 00/18] init: Enable might_sleep() and smp_processor_id()  debugging early Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
    [patch 09/18] cpufreq/pasemi: Adjust system_state check Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
      Re: [patch 09/18] cpufreq/pasemi: Adjust system_state check Viresh Kumar <viresh.kumar@linaro.org> - 2017-05-15 07:30 +0200
        Re: [patch 09/18] cpufreq/pasemi: Adjust system_state check Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 16:40 +0200
    [patch 13/18] extable: Adjust system_state checks Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
      Re: [patch 13/18] extable: Adjust system_state checks Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 16:40 +0200
    [patch 08/18] mm: Adjust system_state check Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
      Re: [patch 08/18] mm: Adjust system_state check Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-15 08:20 +0200
        Re: [patch 08/18] mm: Adjust system_state check Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 16:40 +0200
    [patch 01/18] init: Pin init task to boot cpu initially Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
      Re: [patch 01/18] init: Pin init task to boot cpu initially Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 16:20 +0200
    Re: [patch 12/18] async: Adjust system_state checks Arjan van de Ven <arjan@linux.intel.com> - 2017-05-14 20:50 +0200
    [patch 05/18] metag: Adjust system_state check Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
    [patch 02/18] arm: Adjust system_state check Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
    [patch 18/18] sched: Enable smp_processor_id() checks early Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
      Re: [patch 18/18] sched: Enable smp_processor_id() checks early Steven Rostedt <rostedt@goodmis.org> - 2017-05-15 17:20 +0200
        Re: [patch 18/18] sched: Enable smp_processor_id() checks early Thomas Gleixner <tglx@linutronix.de> - 2017-05-15 17:40 +0200
    [patch 12/18] async: Adjust system_state checks Thomas Gleixner <tglx@linutronix.de> - 2017-05-14 20:50 +0200
    Re: [patch 00/18] init: Enable might_sleep() and smp_processor_id()  debugging early Mark Rutland <mark.rutland@arm.com> - 2017-05-15 14:00 +0200

#1641187 — [patch 00/18] init: Enable might_sleep() and smp_processor_id() debugging early

We recentlty discovered a call path which takes a mutex from the low level
secondary CPU bringup code and wondered why this was not caught by
might_sleep().

The reason is that both debug facilities depend on system_state ==
SYSTEM_RUNNING, which is set after init memory is freed.

That means that the SMP bootup and the builtin driver initialization is not
covered by these checks at all.

The patch series addresses this by adding two intermediate
states. might_sleep() debugging is enabled right when scheduling starts,
i.e. the boot CPU idle task schedules the first time. smp_processor_id()
debugging is enabled right before SMP bringup happens.

Thanks,

	tglx
----
 arch/arm/kernel/smp.c            |    3 +--
 arch/metag/kernel/smp.c          |    3 +--
 arch/x86/kernel/smpboot.c        |    2 +-
 b/arch/arm64/kernel/smp.c        |    3 +--
 b/arch/powerpc/kernel/smp.c      |    2 +-
 drivers/acpi/pci_root.c          |    2 +-
 drivers/base/node.c              |    2 +-
 drivers/cpufreq/pasemi-cpufreq.c |    2 +-
 drivers/iommu/intel-iommu.c      |    4 ++--
 drivers/iommu/of_iommu.c         |    2 +-
 drivers/xen/manage.c             |    2 ++
 include/linux/kernel.h           |    2 ++
 init/main.c                      |   12 ++++++++++--
 kernel/async.c                   |    8 ++++----
 kernel/extable.c                 |    2 +-
 kernel/printk/printk.c           |    2 +-
 kernel/sched/core.c              |    4 +++-
 lib/smp_processor_id.c           |    2 +-
 mm/vmscan.c                      |    2 +-
 19 files changed, 36 insertions(+), 25 deletions(-)

[toc] | [next] | [standalone]

#1641188 — [patch 09/18] cpufreq/pasemi: Adjust system_state check

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

Adjust the system_state check in pas_cpufreq_cpu_exit() to handle the extra
states.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/cpufreq/pasemi-cpufreq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -226,7 +226,7 @@ static int pas_cpufreq_cpu_exit(struct c
 	 * We don't support CPU hotplug. Don't unmap after the system
 	 * has already made it to a running state.
 	 */
-	if (system_state != SYSTEM_BOOTING)
+	if (system_state >= SYSTEM_RUNNING)
 		return 0;
 
 	if (sdcasr_mapbase)

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

#1641285 — Re: [patch 09/18] cpufreq/pasemi: Adjust system_state check

On 14-05-17, 20:27, Thomas Gleixner wrote:
> To enable smp_processor_id() and might_sleep() debug checks earlier, it's
> required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.
> 
> Adjust the system_state check in pas_cpufreq_cpu_exit() to handle the extra
> states.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  drivers/cpufreq/pasemi-cpufreq.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -226,7 +226,7 @@ static int pas_cpufreq_cpu_exit(struct c
>  	 * We don't support CPU hotplug. Don't unmap after the system
>  	 * has already made it to a running state.
>  	 */
> -	if (system_state != SYSTEM_BOOTING)
> +	if (system_state >= SYSTEM_RUNNING)
>  		return 0;
>  
>  	if (sdcasr_mapbase)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

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

#1641760 — Re: [patch 09/18] cpufreq/pasemi: Adjust system_state check

On Mon, 15 May 2017 10:54:03 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 14-05-17, 20:27, Thomas Gleixner wrote:
> > To enable smp_processor_id() and might_sleep() debug checks earlier, it's
> > required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.
> > 
> > Adjust the system_state check in pas_cpufreq_cpu_exit() to handle the extra
> > states.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: linuxppc-dev@lists.ozlabs.org
> > ---
> >  drivers/cpufreq/pasemi-cpufreq.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/drivers/cpufreq/pasemi-cpufreq.c
> > +++ b/drivers/cpufreq/pasemi-cpufreq.c
> > @@ -226,7 +226,7 @@ static int pas_cpufreq_cpu_exit(struct c
> >  	 * We don't support CPU hotplug. Don't unmap after the system
> >  	 * has already made it to a running state.
> >  	 */
> > -	if (system_state != SYSTEM_BOOTING)
> > +	if (system_state >= SYSTEM_RUNNING)
> >  		return 0;
> >  
> >  	if (sdcasr_mapbase)  
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 

This reminds me. My pasemi died. I was trying to see if anyone had an
extra one they could send me ;-)

-- Steve

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

#1641189 — [patch 13/18] extable: Adjust system_state checks

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

Adjust the system_state check in core_kernel_text() to handle the extra
states, i.e. to cover init text up to the point where the system switches
to state RUNNING.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/extable.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -75,7 +75,7 @@ int core_kernel_text(unsigned long addr)
 	    addr < (unsigned long)_etext)
 		return 1;
 
-	if (system_state == SYSTEM_BOOTING &&
+	if (system_state < SYSTEM_RUNNING &&
 	    init_kernel_text(addr))
 		return 1;
 	return 0;

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

#1641754 — Re: [patch 13/18] extable: Adjust system_state checks

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

> To enable smp_processor_id() and might_sleep() debug checks earlier, it's
> required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.
> 
> Adjust the system_state check in core_kernel_text() to handle the extra
> states, i.e. to cover init text up to the point where the system switches
> to state RUNNING.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> ---
>  kernel/extable.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -75,7 +75,7 @@ int core_kernel_text(unsigned long addr)
>  	    addr < (unsigned long)_etext)
>  		return 1;
>  
> -	if (system_state == SYSTEM_BOOTING &&
> +	if (system_state < SYSTEM_RUNNING &&
>  	    init_kernel_text(addr))
>  		return 1;
>  	return 0;
> 

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

#1641190 — [patch 08/18] mm: Adjust system_state check

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

get_nid_for_pfn() checks for system_state == BOOTING to decide whether to
use early_pfn_to_nid() when CONFIG_DEFERRED_STRUCT_PAGE_INIT=y.

That check is dubious, because the switch to state RUNNING happes way after
page_alloc_init_late() has been invoked.

Change the check to less than RUNNING state so it covers the new
intermediate states as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/node.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -377,7 +377,7 @@ static int __ref get_nid_for_pfn(unsigne
 	if (!pfn_valid_within(pfn))
 		return -1;
 #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
-	if (system_state == SYSTEM_BOOTING)
+	if (system_state < SYSTEM_RUNNING)
 		return early_pfn_to_nid(pfn);
 #endif
 	page = pfn_to_page(pfn);

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

#1641300 — Re: [patch 08/18] mm: Adjust system_state check

On Sun, May 14, 2017 at 08:27:24PM +0200, Thomas Gleixner wrote:
> To enable smp_processor_id() and might_sleep() debug checks earlier, it's
> required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.
> 
> get_nid_for_pfn() checks for system_state == BOOTING to decide whether to
> use early_pfn_to_nid() when CONFIG_DEFERRED_STRUCT_PAGE_INIT=y.
> 
> That check is dubious, because the switch to state RUNNING happes way after
> page_alloc_init_late() has been invoked.
> 
> Change the check to less than RUNNING state so it covers the new
> intermediate states as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/node.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -377,7 +377,7 @@ static int __ref get_nid_for_pfn(unsigne
>  	if (!pfn_valid_within(pfn))
>  		return -1;
>  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> -	if (system_state == SYSTEM_BOOTING)
> +	if (system_state < SYSTEM_RUNNING)

Someone better comment the place where these are defined that they need
to be in a specific order, I don't think they have been tested this way
in the past...

With that change:
	Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

thanks,

greg k-h

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

#1641758 — Re: [patch 08/18] mm: Adjust system_state check

On Mon, 15 May 2017 08:18:26 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:


> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -377,7 +377,7 @@ static int __ref get_nid_for_pfn(unsigne
> >  	if (!pfn_valid_within(pfn))
> >  		return -1;
> >  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> > -	if (system_state == SYSTEM_BOOTING)
> > +	if (system_state < SYSTEM_RUNNING)  
> 
> Someone better comment the place where these are defined that they need
> to be in a specific order, I don't think they have been tested this way
> in the past...

Comments never hurt but it has been used before this patch set.

$ git grep  SYSTEM_RUNNING v4.12-rc1 | grep -e '<' -e '>'
v4.12-rc1:arch/powerpc/kernel/smp.c:    if (system_state < SYSTEM_RUNNING)
v4.12-rc1:arch/powerpc/platforms/powernv/eeh-powernv.c:         if (system_state < SYSTEM_RUNNING)
v4.12-rc1:arch/powerpc/platforms/powernv/eeh-powernv.c:         if (system_state < SYSTEM_RUNNING)
v4.12-rc1:drivers/cpuidle/cpuidle-powernv.c:    if (unlikely(system_state < SYSTEM_RUNNING))
v4.12-rc1:drivers/xen/xenbus/xenbus_probe.c:    if (system_state > SYSTEM_RUNNING) {

-- Steve

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

#1641191 — [patch 01/18] init: Pin init task to boot cpu initially

Some of the boot code in init_kernel_freeable() which runs before SMP
bringup assumes (rightfully) that it runs on the boot cpu and therefor can
use smp_processor_id() in preemptible context.

That works so far because the smp_processor_id() check starts to be
effective after smp bringup. That's just wrong. Starting with SMP bringup
and the ability to move threads around, smp_processor_id() in preemptible
context is broken.

Aside of that it does not make sense to allow init to run on all cpus
before sched_smp_init() has been run.

Pin the init to the boot cpu so the existing code can continue to use
smp_processor_id() without triggering the checks when the enabling of those
checks starts earlier.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 init/main.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- a/init/main.c
+++ b/init/main.c
@@ -1015,10 +1015,13 @@ static noinline void __init kernel_init_
 	 * init can allocate pages on any node
 	 */
 	set_mems_allowed(node_states[N_MEMORY]);
+
 	/*
-	 * init can run on any cpu.
+	 * Pin init on the boot cpu. Task migration is not properly working
+	 * until sched_init_smp() has been run. It will set the allowed
+	 * cpus for init to the non isolated cpus.
 	 */
-	set_cpus_allowed_ptr(current, cpu_all_mask);
+	set_cpus_allowed_ptr(current, cpumask_of(raw_smp_processor_id()));
 
 	cad_pid = task_pid(current);
 

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

#1641731 — Re: [patch 01/18] init: Pin init task to boot cpu initially

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

> Some of the boot code in init_kernel_freeable() which runs before SMP
> bringup assumes (rightfully) that it runs on the boot cpu and therefor can
> use smp_processor_id() in preemptible context.
> 
> That works so far because the smp_processor_id() check starts to be
> effective after smp bringup. That's just wrong. Starting with SMP bringup
> and the ability to move threads around, smp_processor_id() in preemptible
> context is broken.
> 
> Aside of that it does not make sense to allow init to run on all cpus
> before sched_smp_init() has been run.
> 
> Pin the init to the boot cpu so the existing code can continue to use
> smp_processor_id() without triggering the checks when the enabling of those
> checks starts earlier.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  init/main.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1015,10 +1015,13 @@ static noinline void __init kernel_init_
>  	 * init can allocate pages on any node
>  	 */
>  	set_mems_allowed(node_states[N_MEMORY]);
> +
>  	/*
> -	 * init can run on any cpu.
> +	 * Pin init on the boot cpu. Task migration is not properly working
> +	 * until sched_init_smp() has been run. It will set the allowed
> +	 * cpus for init to the non isolated cpus.
>  	 */
> -	set_cpus_allowed_ptr(current, cpu_all_mask);
> +	set_cpus_allowed_ptr(current, cpumask_of(raw_smp_processor_id()));
>  
>  	cad_pid = task_pid(current);
>  
> 

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

#1641192 — Re: [patch 12/18] async: Adjust system_state checks

On 5/14/2017 11:27 AM, Thomas Gleixner wrote:

looks good .. ack

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

#1641193 — [patch 05/18] metag: Adjust system_state check

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

Adjust the system_state check in stop_this_cpu() to handle the extra states.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: linux-metag@vger.kernel.org
---
 arch/metag/kernel/smp.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/arch/metag/kernel/smp.c
+++ b/arch/metag/kernel/smp.c
@@ -567,8 +567,7 @@ static void stop_this_cpu(void *data)
 {
 	unsigned int cpu = smp_processor_id();
 
-	if (system_state == SYSTEM_BOOTING ||
-	    system_state == SYSTEM_RUNNING) {
+	if (system_state <= SYSTEM_RUNNING) {
 		spin_lock(&stop_lock);
 		pr_crit("CPU%u: stopping\n", cpu);
 		dump_stack();

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

#1641194 — [patch 02/18] arm: Adjust system_state check

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

Adjust the system_state check in ipi_cpu_stop() to handle the extra states.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
---
 arch/arm/kernel/smp.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -555,8 +555,7 @@ static DEFINE_RAW_SPINLOCK(stop_lock);
  */
 static void ipi_cpu_stop(unsigned int cpu)
 {
-	if (system_state == SYSTEM_BOOTING ||
-	    system_state == SYSTEM_RUNNING) {
+	if (system_state <= SYSTEM_RUNNING) {
 		raw_spin_lock(&stop_lock);
 		pr_crit("CPU%u: stopping\n", cpu);
 		dump_stack();

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

#1641195 — [patch 18/18] sched: Enable smp_processor_id() checks early

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

Enable it right before the first non-boot CPU is brought up.

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


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

--- a/init/main.c
+++ b/init/main.c
@@ -1025,6 +1025,9 @@ static noinline void __init kernel_init_
 	 */
 	set_cpus_allowed_ptr(current, cpumask_of(raw_smp_processor_id()));
 
+	/* Enable smp_processor_id() checks */
+	system_state = SYSTEM_BOOTING_SMP;
+
 	cad_pid = task_pid(current);
 
 	smp_prepare_cpus(setup_max_cpus);
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -28,7 +28,7 @@ notrace static unsigned int check_preemp
 	/*
 	 * It is valid to assume CPU-locality during early bootup:
 	 */
-	if (system_state != SYSTEM_RUNNING)
+	if (system_state < SYSTEM_BOOTING_SMP)
 		goto out;
 
 	/*

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

#1641799 — Re: [patch 18/18] sched: Enable smp_processor_id() checks early

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

> smp_processor_id() checks are enabled after the boot process is done. That
> hides bugs in the smp bringup and driver initialization code.
> 
> Enable it right before the first non-boot CPU is brought up.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  init/main.c            |    3 +++
>  lib/smp_processor_id.c |    2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  init/main.c            |    3 +++
>  lib/smp_processor_id.c |    2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1025,6 +1025,9 @@ static noinline void __init kernel_init_
>  	 */
>  	set_cpus_allowed_ptr(current, cpumask_of(raw_smp_processor_id()));
>  
> +	/* Enable smp_processor_id() checks */
> +	system_state = SYSTEM_BOOTING_SMP;
> +

Just a nit, but why set this here and not just before
smp_prepare_cpus()? Anyway...


Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


>  	cad_pid = task_pid(current);
>  
>  	smp_prepare_cpus(setup_max_cpus);
> --- a/lib/smp_processor_id.c
> +++ b/lib/smp_processor_id.c
> @@ -28,7 +28,7 @@ notrace static unsigned int check_preemp
>  	/*
>  	 * It is valid to assume CPU-locality during early bootup:
>  	 */
> -	if (system_state != SYSTEM_RUNNING)
> +	if (system_state < SYSTEM_BOOTING_SMP)
>  		goto out;
>  
>  	/*
> 

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

#1641822 — Re: [patch 18/18] sched: Enable smp_processor_id() checks early

On Mon, 15 May 2017, Steven Rostedt wrote:

> On Sun, 14 May 2017 20:27:34 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > smp_processor_id() checks are enabled after the boot process is done. That
> > hides bugs in the smp bringup and driver initialization code.
> > 
> > Enable it right before the first non-boot CPU is brought up.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  init/main.c            |    3 +++
> >  lib/smp_processor_id.c |    2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  init/main.c            |    3 +++
> >  lib/smp_processor_id.c |    2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1025,6 +1025,9 @@ static noinline void __init kernel_init_
> >  	 */
> >  	set_cpus_allowed_ptr(current, cpumask_of(raw_smp_processor_id()));
> >  
> > +	/* Enable smp_processor_id() checks */
> > +	system_state = SYSTEM_BOOTING_SMP;
> > +
> 
> Just a nit, but why set this here and not just before
> smp_prepare_cpus()? Anyway...

Because smp_prepare_cpus() is full of "UP" assumptions. Thinking more about
it, we just can set the cpus allowed ptr right after creating it and just
have a single state which enables both checks. I'll have a look.

Thanks,

	tglx

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

#1641196 — [patch 12/18] async: Adjust system_state checks

To enable smp_processor_id() and might_sleep() debug checks earlier, it's
required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.

Adjust the system_state check in async_run_entry_fn() and
async_synchronize_cookie_domain() to handle the extra states.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
---
 kernel/async.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/kernel/async.c
+++ b/kernel/async.c
@@ -114,14 +114,14 @@ static void async_run_entry_fn(struct wo
 	ktime_t uninitialized_var(calltime), delta, rettime;
 
 	/* 1) run (and print duration) */
-	if (initcall_debug && system_state == SYSTEM_BOOTING) {
+	if (initcall_debug && system_state < SYSTEM_RUNNING) {
 		pr_debug("calling  %lli_%pF @ %i\n",
 			(long long)entry->cookie,
 			entry->func, task_pid_nr(current));
 		calltime = ktime_get();
 	}
 	entry->func(entry->data, entry->cookie);
-	if (initcall_debug && system_state == SYSTEM_BOOTING) {
+	if (initcall_debug && system_state < SYSTEM_RUNNING) {
 		rettime = ktime_get();
 		delta = ktime_sub(rettime, calltime);
 		pr_debug("initcall %lli_%pF returned 0 after %lld usecs\n",
@@ -284,14 +284,14 @@ void async_synchronize_cookie_domain(asy
 {
 	ktime_t uninitialized_var(starttime), delta, endtime;
 
-	if (initcall_debug && system_state == SYSTEM_BOOTING) {
+	if (initcall_debug && system_state < SYSTEM_RUNNING) {
 		pr_debug("async_waiting @ %i\n", task_pid_nr(current));
 		starttime = ktime_get();
 	}
 
 	wait_event(async_done, lowest_in_progress(domain) >= cookie);
 
-	if (initcall_debug && system_state == SYSTEM_BOOTING) {
+	if (initcall_debug && system_state < SYSTEM_RUNNING) {
 		endtime = ktime_get();
 		delta = ktime_sub(endtime, starttime);
 

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

#1641584

Hi Thomas,

On Sun, May 14, 2017 at 08:27:16PM +0200, Thomas Gleixner wrote:
> We recentlty discovered a call path which takes a mutex from the low level
> secondary CPU bringup code and wondered why this was not caught by
> might_sleep().
> 
> The reason is that both debug facilities depend on system_state ==
> SYSTEM_RUNNING, which is set after init memory is freed.
> 
> That means that the SMP bootup and the builtin driver initialization is not
> covered by these checks at all.
> 
> The patch series addresses this by adding two intermediate
> states. might_sleep() debugging is enabled right when scheduling starts,
> i.e. the boot CPU idle task schedules the first time. smp_processor_id()
> debugging is enabled right before SMP bringup happens.

Thanks for putting this together!

I've tested the series on a Juno R1 (arm64) system, with both DT and
ACPI.

Patch 17 finds the arm64 mutex issue [1] we discussed previously.

Otherwise, no new issues spotted with this on arm64, just an (existing)
ACPI PMU issue for which I've evidently not run sufficient checks on.

So for the bits relevant to arm64 (patches 1, 3, 7, 8, and 12-18):

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[1] https://lkml.kernel.org/r/20170511125430.GD14766@leverpostej

> 
> Thanks,
> 
> 	tglx
> ----
>  arch/arm/kernel/smp.c            |    3 +--
>  arch/metag/kernel/smp.c          |    3 +--
>  arch/x86/kernel/smpboot.c        |    2 +-
>  b/arch/arm64/kernel/smp.c        |    3 +--
>  b/arch/powerpc/kernel/smp.c      |    2 +-
>  drivers/acpi/pci_root.c          |    2 +-
>  drivers/base/node.c              |    2 +-
>  drivers/cpufreq/pasemi-cpufreq.c |    2 +-
>  drivers/iommu/intel-iommu.c      |    4 ++--
>  drivers/iommu/of_iommu.c         |    2 +-
>  drivers/xen/manage.c             |    2 ++
>  include/linux/kernel.h           |    2 ++
>  init/main.c                      |   12 ++++++++++--
>  kernel/async.c                   |    8 ++++----
>  kernel/extable.c                 |    2 +-
>  kernel/printk/printk.c           |    2 +-
>  kernel/sched/core.c              |    4 +++-
>  lib/smp_processor_id.c           |    2 +-
>  mm/vmscan.c                      |    2 +-
>  19 files changed, 36 insertions(+), 25 deletions(-)
> 
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web