Re: [Xen-devel] [PATCH 2/4] xen/x86: Drop erronious barriers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/12/2016 20:32, Stefano Stabellini wrote:
> > On Tue, 6 Dec 2016, Stefano Stabellini wrote:
> > > 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.cooper3@xxxxxxxxxx>
> > > > > > ---
> > > > > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > > > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > > > > 
> > > > > > 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.
> > 
> > Just to be clear, on ARM the barriers are unneeded only if it is
> > unimportant that "init stuff" (which correspond to all the
> > initialization done in start_secondary up to smp_wmb) below is completed
> > before "write cpu_online_map". But it looks like we do want to complete
> > mmu, irq, timer initializations and set the current vcpu before marking
> > the cpu as online, right?
> 
> *mb are memory barriers. This would not prevent write to system registers and
> co-processor registers happening before "write cpu_online_map". Only an
> dsb(sy); isb() would ensure this.
> 
> However, I don't think we really care about the state of the hardware for a
> specific CPU. This should never be accessed by another one.
> 
> > 
> > > 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?
> 
> I don't think so. If synchronization of hardware access was necessary it would
> have been taken care by the driver itself.

I thought the same, thanks for confirming.


> What we should care here is if there any xen internal state that are required
> between CPU0 and CPU1.
> 
> In this specific case, I think we should have the smp_wmb() barrier to ensure
> all write happening whilst calling local notifiers will be visible before the
> CPU mark itself as online. So IHMO, the patch looks good to me (see a small
> comment below).

Andrew thinks that (on x86) there is actually nothing that CPU0 will be
looking at, that has been set by CPU1. However looking at the code it is
difficult to verify. There are many cpu notifiers and many things
written by start_secondary. I would prefer to submit this patch, and be
safe.


> > > 
> > > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > 
> > > 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();
> 
> It would be good to start annotating the pair of barrier with "This barrier
> corresponds with the one in...". It would avoid "wild" barrier in the code :).
> 
> > > 
> > >      /*
> > >       * Nuke start of day info before checking one last time if the CPU
> > > 
> 
> Regards,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel