[PATCH] ia64: make cpu_callin_map non-volatile.

[PATCH] ia64: make cpu_callin_map non-volatile.

[Luck, Tony]

From: Rusty Russell <rusty@rustcorp.com.au>

cpumask_test_cpu() doesn't take volatile, unlike the obsoleted
cpu_isset.  The only place ia64 really cares is the spin waiting for a
bit; udelay() is probably a barrier but insert rmb() to be sure.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/kernel/smpboot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/kernel/smpboot.c b/arch/ia64/kernel/smpboot.c
index 15051e9c2c6f..c55afa7f5723 100644
--- a/arch/ia64/kernel/smpboot.c
+++ b/arch/ia64/kernel/smpboot.c
@@ -127,7 +127,7 @@ int smp_num_siblings = 1;
 volatile int ia64_cpu_to_sapicid[NR_CPUS];
 EXPORT_SYMBOL(ia64_cpu_to_sapicid);
 
-static volatile cpumask_t cpu_callin_map;
+static cpumask_t cpu_callin_map;
 
 struct smp_boot_data smp_boot_data __initdata;
 
@@ -477,6 +477,7 @@ do_boot_cpu (int sapicid, int cpu, struct task_struct *idle)
 	for (timeout = 0; timeout < 100000; timeout++) {
 		if (cpumask_test_cpu(cpu, &cpu_callin_map))
 			break;  /* It has booted */
+		rmb(); /* Make sure we re-read cpu_callin_map */
 		udelay(100);
 	}
 	Dprintk("\n");
-- 
2.1.4

Re: [PATCH] ia64: make cpu_callin_map non-volatile.

[Linus Torvalds]

On Tue, May 26, 2015 at 6:18 PM, Luck, Tony <tony.luck@intel.com> wrote:
>
> cpumask_test_cpu() doesn't take volatile, unlike the obsoleted
> cpu_isset.  The only place ia64 really cares is the spin waiting for a
> bit; udelay() is probably a barrier but insert rmb() to be sure.

Hmm. udelay() definitely should be a barrier, so this patch shouldn't
matter. But even if it weren't, rmb() would be the wrong barrier to
use here.

rmb() is a "CPU or IO read barrier" to order reads wrt other cores or
DMA, and needs to be paired with a wmb() on other cores (and for DMA
it is assumed that there is sufficiently write ordering for the DMA
itself).

If it's about just other CPU's, it would be "smp_rmb()", which is
often much cheaper (due to no IO serialization, particularly on
POWER).

But in this case it doesn't seem to be about other agents at all
(CPU's or DMA), you literally are just looking for a compiler barrier
to make sure the read doesn't get moved out of the loop. And the
function for that is just "barrier()".

So I'm skipping this patch, because I dont' think it changes anything
real, and from a "let's be careful" standpoint it's actually doing the
wrong thing.

                       Linus

Re: [PATCH] ia64: make cpu_callin_map non-volatile.

[Rusty Russell]

Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Tue, May 26, 2015 at 6:18 PM, Luck, Tony <tony.luck@intel.com> wrote:
>>
>> cpumask_test_cpu() doesn't take volatile, unlike the obsoleted
>> cpu_isset.  The only place ia64 really cares is the spin waiting for a
>> bit; udelay() is probably a barrier but insert rmb() to be sure.
>
> Hmm. udelay() definitely should be a barrier, so this patch shouldn't
> matter. But even if it weren't, rmb() would be the wrong barrier to
> use here.
>
> rmb() is a "CPU or IO read barrier" to order reads wrt other cores or
> DMA, and needs to be paired with a wmb() on other cores (and for DMA
> it is assumed that there is sufficiently write ordering for the DMA
> itself).
>
> If it's about just other CPU's, it would be "smp_rmb()", which is
> often much cheaper (due to no IO serialization, particularly on
> POWER).
>
> But in this case it doesn't seem to be about other agents at all
> (CPU's or DMA), you literally are just looking for a compiler barrier
> to make sure the read doesn't get moved out of the loop. And the
> function for that is just "barrier()".
>
> So I'm skipping this patch, because I dont' think it changes anything
> real, and from a "let's be careful" standpoint it's actually doing the
> wrong thing.

Good point: the fact that other cpus are the ones setting the bit is a
red herring here.

Thanks,
Rusty.

From: Rusty Russell <rusty@rustcorp.com.au>
Subject: ia64: make cpu_callin_map non-volatile.

cpumask_test_cpu() doesn't take volatile, unlike the obsoleted
cpu_isset.  The only place ia64 really cares is the spin waiting for a
bit; udelay() is probably a barrier but insert barrier() to be sure.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/arch/ia64/kernel/smpboot.c b/arch/ia64/kernel/smpboot.c
index 15051e9c2c6f..629975b56608 100644
--- a/arch/ia64/kernel/smpboot.c
+++ b/arch/ia64/kernel/smpboot.c
@@ -127,7 +127,7 @@ int smp_num_siblings = 1;
 volatile int ia64_cpu_to_sapicid[NR_CPUS];
 EXPORT_SYMBOL(ia64_cpu_to_sapicid);
 
-static volatile cpumask_t cpu_callin_map;
+static cpumask_t cpu_callin_map;
 
 struct smp_boot_data smp_boot_data __initdata;
 
@@ -477,6 +477,7 @@ do_boot_cpu (int sapicid, int cpu, struct task_struct *idle)
 	for (timeout = 0; timeout < 100000; timeout++) {
 		if (cpumask_test_cpu(cpu, &cpu_callin_map))
 			break;  /* It has booted */
+		barrier(); /* Make sure we re-read cpu_callin_map */
 		udelay(100);
 	}
 	Dprintk("\n");