On Tue, 6 Dec 2016, Andrew Cooper wrote:
> On 05/12/2016 19:17, Stefano Stabellini wrote:
> > On Mon, 5 Dec 2016, Andrew Cooper wrote:
> >> None of these barriers serve any purpose, as they are not synchronising
> >> with
> >> any anything on remote CPUs.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> >> ---
> >> CC: Jan Beulich <jbeul...@suse.com>
> >> CC: Stefano Stabellini <sstabell...@kernel.org>
> >> CC: Julien Grall <julien.gr...@arm.com>
> >>
> >> Restricting to just the $ARCH maintainers, as this is a project-wide sweep.
> >>
> >> Julien/Stefano: I think the ARM smpboot inhereted the erronious barrier
> >> usage
> >> from x86, but I don't know whether further development has gained a
> >> dependence
> >> on them.
> > We turned them into smp_wmb already (kudos to IanC).
>
> Right, but the entire point I am trying to argue is that they are not
> needed in the first place.
This is the current code:
CPU 1 CPU 0
----- -----
init stuff read cpu_online_map
write barrier
write cpu_online_map do more initialization
write barrier
init more stuff
I agree that it's wrong, because the second write barrier in
start_secondary is useless and in addition we are missing a read barrier
in __cpu_up, corresponding to the first write barrier in
start_secondary.
I think it should look like:
CPU 1 CPU 0
----- -----
init stuff read cpu_online_map
write barrier read barrier
write cpu_online_map do more initialization
init more stuff
The patch is as follow.
Julien, what do you think?
Also, do we need to change the remaming smp_wmb() in start_secondary to
wmb() to ensure execution ordering as well as memory access ordering?
Signed-off-by: Stefano Stabellini <sstabell...@kernel.org>
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 90ad1d0..c841a15 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -311,7 +311,6 @@ void start_secondary(unsigned long boot_phys_offset,
/* Now report this CPU is up */
cpumask_set_cpu(cpuid, &cpu_online_map);
- smp_wmb();
local_irq_enable();
local_abort_enable();
@@ -408,6 +407,7 @@ int __cpu_up(unsigned int cpu)
cpu_relax();
process_pending_softirqs();
}
+ smp_rmb();
/*
* Nuke start of day info before checking one last time if the CPU
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
