[PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[Atish Patra]

Currently, smp_fill_in_cpu_possible_map() unncessarily iterates
over all cpus to update possible/present cpumask based on nr_cpu_ids.
nr_cpus can be supported if nr_cpu_ids is checked instead of NR_CPUS
during MD parsing as well and saves some tiny boot time as well.

Update possible/present masks during MD parsing based on nr_cpu_ids.

Signed-off-by: Atish Patra <atish.patra@oracle.com>
Reviewed-by: Anthony Yznaga <anthony.yznaga@oracle.com>
---
 arch/sparc/include/asm/smp_64.h |    2 --
 arch/sparc/kernel/mdesc.c       |   16 ++++++++--------
 arch/sparc/kernel/prom_64.c     |   16 ++++++++++++----
 arch/sparc/kernel/setup_64.c    |    1 -
 arch/sparc/kernel/smp_64.c      |   14 --------------
 5 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/arch/sparc/include/asm/smp_64.h b/arch/sparc/include/asm/smp_64.h
index ce2233f..26d9e77 100644
--- a/arch/sparc/include/asm/smp_64.h
+++ b/arch/sparc/include/asm/smp_64.h
@@ -43,7 +43,6 @@ void arch_send_call_function_ipi_mask(const struct cpumask *mask);
 int hard_smp_processor_id(void);
 #define raw_smp_processor_id() (current_thread_info()->cpu)
 
-void smp_fill_in_cpu_possible_map(void);
 void smp_fill_in_sib_core_maps(void);
 void cpu_play_dead(void);
 
@@ -73,7 +72,6 @@ void __cpu_die(unsigned int cpu);
 #define smp_fill_in_sib_core_maps() do { } while (0)
 #define smp_fetch_global_regs() do { } while (0)
 #define smp_fetch_global_pmu() do { } while (0)
-#define smp_fill_in_cpu_possible_map() do { } while (0)
 
 #endif /* !(CONFIG_SMP) */
 
diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index 1122886..51f5e73 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -558,10 +558,10 @@ static void __init report_platform_properties(void)
 
 		if (v) {
 			max_cpu = *v;
-			if (max_cpu > NR_CPUS)
-				max_cpu = NR_CPUS;
+			if (max_cpu > nr_cpu_ids)
+				max_cpu = nr_cpu_ids;
 		} else {
-			max_cpu = NR_CPUS;
+			max_cpu = nr_cpu_ids;
 		}
 		for (i = 0; i < max_cpu; i++)
 			set_cpu_possible(i, true);
@@ -770,7 +770,7 @@ static void mark_proc_ids(struct mdesc_handle *hp, u64 mp, int proc_id)
 			continue;
 
 		id = mdesc_get_property(hp, t, "id", NULL);
-		if (*id < NR_CPUS)
+		if (*id < nr_cpu_ids)
 			cpu_data(*id).proc_id = proc_id;
 	}
 }
@@ -861,12 +861,12 @@ static void *mdesc_iterate_over_cpus(void *(*func)(struct mdesc_handle *, u64, i
 		int cpuid = *id;
 
 #ifdef CONFIG_SMP
-		if (cpuid >= NR_CPUS) {
-			printk(KERN_WARNING "Ignoring CPU %d which is "
-			       ">= NR_CPUS (%d)\n",
-			       cpuid, NR_CPUS);
+		if (cpuid >= nr_cpu_ids) {
+			pr_warn("Ignoring CPU %d which is >= nr_cpu_ids (%d)\n",
+				cpuid, nr_cpu_ids);
 			continue;
 		}
+
 		if (!cpumask_test_cpu(cpuid, mask))
 			continue;
 #endif
diff --git a/arch/sparc/kernel/prom_64.c b/arch/sparc/kernel/prom_64.c
index 20cc5d8..34775f7 100644
--- a/arch/sparc/kernel/prom_64.c
+++ b/arch/sparc/kernel/prom_64.c
@@ -448,10 +448,9 @@ static void *of_iterate_over_cpus(void *(*func)(struct device_node *, int, int),
 			prom_halt();
 		}
 #ifdef CONFIG_SMP
-		if (cpuid >= NR_CPUS) {
-			printk(KERN_WARNING "Ignoring CPU %d which is "
-			       ">= NR_CPUS (%d)\n",
-			       cpuid, NR_CPUS);
+		if (cpuid >= nr_cpu_ids) {
+			pr_warn("Ignoring CPU %d which is >= nr_cpu_ids (%d)\n",
+				cpuid, nr_cpu_ids);
 			continue;
 		}
 #endif
@@ -491,6 +490,15 @@ void __init of_populate_present_mask(void)
 
 	ncpus_probed = 0;
 	of_iterate_over_cpus(record_one_cpu, 0);
+
+	/* This is a hack to accommodate the assumption in timer code that CPU0
+	 * always exists and therefore timers can be assigned to CPU0 at static
+	 * initialization time.  Setting CPU0 in the possible mask ensures that
+	 * the per-cpu timer data structures for CPU0 are properly initialized
+	 * for use by such timers even when there is no CPU0 present.
+	 */
+	if (!cpu_possible(0))
+		set_cpu_possible(0, true);
 }
 
 static void *fill_in_one_cpu(struct device_node *dp, int cpuid, int arg)
diff --git a/arch/sparc/kernel/setup_64.c b/arch/sparc/kernel/setup_64.c
index 6b7331d..9ce259a 100644
--- a/arch/sparc/kernel/setup_64.c
+++ b/arch/sparc/kernel/setup_64.c
@@ -669,7 +669,6 @@ void __init setup_arch(char **cmdline_p)
 
 	paging_init();
 	init_sparc64_elf_hwcap();
-	smp_fill_in_cpu_possible_map();
 	/*
 	 * Once the OF device tree and MDESC have been setup and nr_cpus has
 	 * been parsed, we know the list of possible cpus.  Therefore we can
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
index d3035ba..8a6151a 100644
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -1227,20 +1227,6 @@ void __init smp_setup_processor_id(void)
 		xcall_deliver_impl = hypervisor_xcall_deliver;
 }
 
-void __init smp_fill_in_cpu_possible_map(void)
-{
-	int possible_cpus = num_possible_cpus();
-	int i;
-
-	if (possible_cpus > nr_cpu_ids)
-		possible_cpus = nr_cpu_ids;
-
-	for (i = 0; i < possible_cpus; i++)
-		set_cpu_possible(i, true);
-	for (; i < NR_CPUS; i++)
-		set_cpu_possible(i, false);
-}
-
 void smp_fill_in_sib_core_maps(void)
 {
 	unsigned int i;
-- 
1.7.1

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[David Miller]

So, this is fixing a regression, right?

If so, you need to provide a proper "Fixes: " tag identifying
the commit that introduced the bug.

I'm also finding it hard to believe that forcing cpu 0 as a possible
cpu is required.  Have you considered contacting the timer subsystem
maintainers and asked them to remove this assumption?

Thank you.

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[Atish Patra]

On 11/19/2017 12:54 AM, David Miller wrote:
> So, this is fixing a regression, right?
Yes.
> If so, you need to provide a proper "Fixes: " tag identifying
> the commit that introduced the bug.
Sure. I will add that in v2.
> I'm also finding it hard to believe that forcing cpu 0 as a possible
> cpu is required.  Have you considered contacting the timer subsystem
> maintainers and asked them to remove this assumption?
I will check with the timer maintainers.

Regards,
Atish
> Thank you.

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[Atish Patra]

Hi Thomas,
I am looking at a spinlock bad magic issue in timer code on a legacy 
sparc machine.

0eeda71b (timer: Replace timer base by a cpu index)
As per my understanding, the above patch assumes there is always a cpu 0 
and that timer to CPU0 should be assigned statically to avoid 
boot_tvec_base logic.
DEFINE_TIMER macro seems to initialize the flags to 0 and the commit 
text also points to that.
Please let me know if I missed something.

We observed the spinlock bad magic with legacy sun4u machines which have 
sparse numbered cpus and does not have cpu0.
We may be wrong but our investigation pointed out that per-cpu 
timer_base structure for cpu 0 is not initialized leading invalid
magic value. console_timer seems to be statically assigned to cpu0 in 
this case.

-----------------------------------part of the relevant 
dmesg---------------------------------------------------------------------------
[  179.005589] clocksource: tick: mask: 0xffffffffffffffff max_cycles: 
0x5c4093a7d1, max_idle_ns: 440795210635 ns
[  179.109253] clocksource: mult[2800000] shift[24]
[  179.148828] clockevent: mult[66666666] shift[32]
[  179.190350] BUG: spinlock bad magic on CPU#6, swapper/6/0
[  179.190392]  lock: timer_bases+0x0/0x2180, .magic: 00000000, .owner: 
<none>/-1, .owner_cpu: 0
[  179.190409] CPU: 6 PID: 0 Comm: swapper/6 Not tainted 4.8.0-dirty #92
[  179.190416] Call Trace:
[  179.190445]  [00000000004abe60] spin_dump+0x60/0xa0
[  179.190460]  [00000000004abeb8] spin_bug+0x18/0x40
[  179.190476]  [00000000004ac024] do_raw_spin_lock+0x24/0x120
[  179.190510]  [00000000008c6088] _raw_spin_lock_irqsave+0x48/0x60
[  179.190530]  [00000000004bfd08] lock_timer_base.isra.13+0x68/0xc0
[  179.190545]  [00000000004c07cc] mod_timer+0xcc/0x2c0
[  179.190562]  [0000000000a6928c] con_init+0x120/0x28c
[  179.190575]  [0000000000a68a4c] console_init+0x1c/0x38
[  179.190592]  [0000000000a4e92c] start_kernel+0x31c/0x41c
[  179.190607]  [0000000000a500fc] start_early_boot+0x274/0x284
[  179.190622]  [00000000008bed2c] tlb_fixup_done+0x4c/0x60
[  179.190632]  [0000000000000000]           (null)
---------------------------------------------------------------------------------------------------------------------------------------------------------------

I tried to avoid this by setting the cpu0 in cpu possible map just for 
sun4u.
Here is upstream discussion for my patch:
http://www.spinics.net/lists/sparclinux/msg19144.html
I agree that this is a hack. I would prefer any other elegant approach 
to solve this issue.

Regards,
Atish

On 11/27/2017 10:39 AM, Atish Patra wrote:
>
>
> On 11/19/2017 12:54 AM, David Miller wrote:
>> So, this is fixing a regression, right?
> Yes.
>> If so, you need to provide a proper "Fixes: " tag identifying
>> the commit that introduced the bug.
> Sure. I will add that in v2.
>> I'm also finding it hard to believe that forcing cpu 0 as a possible
>> cpu is required.  Have you considered contacting the timer subsystem
>> maintainers and asked them to remove this assumption?
> I will check with the timer maintainers.
>
> Regards,
> Atish
>> Thank you.
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[David Miller]

From: Atish Patra <atish.patra@oracle.com>
Date: Tue, 5 Dec 2017 19:41:47 -0600

> I am looking at a spinlock bad magic issue in timer code on a legacy
> sparc machine.
> 
> 0eeda71b (timer: Replace timer base by a cpu index)
> As per my understanding, the above patch assumes there is always a cpu
> 0 and that timer to CPU0 should be assigned statically to avoid
> boot_tvec_base logic.
> DEFINE_TIMER macro seems to initialize the flags to 0 and the commit
> text also points to that.
> Please let me know if I missed something.
> 
> We observed the spinlock bad magic with legacy sun4u machines which
> have sparse numbered cpus and does not have cpu0.
> We may be wrong but our investigation pointed out that per-cpu
> timer_base structure for cpu 0 is not initialized leading invalid
> magic value. console_timer seems to be statically assigned to cpu0 in
> this case.

Yeah, you can't really assume that cpu 0 is the boot cpu let alone
even exists.  And it is clear that the timer code now has such a
dependency.

Thomas, you'll probably have to do something like have a fixup early
in the boot process which fixes up the flags field of all of the
statically defined timer objects so that it contains the cpu that
is running the early bootup process.

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[Atish Patra]

ping ?

Regards
On 12/08/2017 12:13 PM, David Miller wrote:
> From: Atish Patra <atish.patra@oracle.com>
> Date: Tue, 5 Dec 2017 19:41:47 -0600
>
>> I am looking at a spinlock bad magic issue in timer code on a legacy
>> sparc machine.
>>
>> 0eeda71b (timer: Replace timer base by a cpu index)
>> As per my understanding, the above patch assumes there is always a cpu
>> 0 and that timer to CPU0 should be assigned statically to avoid
>> boot_tvec_base logic.
>> DEFINE_TIMER macro seems to initialize the flags to 0 and the commit
>> text also points to that.
>> Please let me know if I missed something.
>>
>> We observed the spinlock bad magic with legacy sun4u machines which
>> have sparse numbered cpus and does not have cpu0.
>> We may be wrong but our investigation pointed out that per-cpu
>> timer_base structure for cpu 0 is not initialized leading invalid
>> magic value. console_timer seems to be statically assigned to cpu0 in
>> this case.
> Yeah, you can't really assume that cpu 0 is the boot cpu let alone
> even exists.  And it is clear that the timer code now has such a
> dependency.
>
> Thomas, you'll probably have to do something like have a fixup early
> in the boot process which fixes up the flags field of all of the
> statically defined timer objects so that it contains the cpu that
> is running the early bootup process.
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sparc64: Set possible and present masks based on nr_cpu_ids

[David Miller]

From: Atish Patra <atish.patra@oracle.com>
Date: Wed, 10 Jan 2018 08:51:09 -0600

> ping ?

Thomas and co. are overloaded working on Meltdown and Spectre, so don't
expect a reply from them on this for at least a few more weeks at best.