Re: [Xen-devel] [PATCH v2] xen/arm: fix smpboot barriers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/12/2016 19:02, Stefano Stabellini wrote:
> > Remove useless smp_wmb() barrier after cpumask_set_cpu(cpuid,
> > &cpu_online_map), which is not synchronizing against anything.
> > 
> > Keep the other smp_wmb(), before the cpumask_set_cpu call, to ensure
> > that all writes before setting the cpu online are visible to other cpus.
> > For that to work properly, we need a corresponding smp_rmb() barrier,
> > after reading the online cpumask from other processors, which is
> > currently missing. Add it.
> > 
> > See: http://marc.info/?l=xen-devel&m=148093236307211
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> With some minor coding styles change:
> 
> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> 
> Also, I am wondering whether we should backport this patch? Technically the
> barrier are wrong.

It is true that the missing smp_rmb() could cause problems. However this
is the kind of patch that would be better left stirring in staging for a
few weeks before backporting.


> > ---
> > 
> > Changes in v2:
> > - add in-code comments
> > 
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 90ad1d0..4570f45 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -307,11 +307,12 @@ void start_secondary(unsigned long boot_phys_offset,
> > 
> >      /* Run local notifiers */
> >      notify_cpu_starting(cpuid);
> > +    /* Ensure that previous writes are visible before marking the cpu as
> > +     * online. */
> 
> It looks like smpboot.c is using different style for comment. However, the
> correct one is
> 
> /*
>  * Ensure...
>  * ...
>  */
> 
> >      smp_wmb();
> > 
> >      /* Now report this CPU is up */
> >      cpumask_set_cpu(cpuid, &cpu_online_map);
> > -    smp_wmb();
> > 
> >      local_irq_enable();
> >      local_abort_enable();
> > @@ -408,6 +409,9 @@ int __cpu_up(unsigned int cpu)
> >          cpu_relax();
> >          process_pending_softirqs();
> >      }
> > +    /* Ensure that other cpus' initializations are visible before
> > +     * proceeding. Corresponds to smp_wmb() in start_secondary. */
> 
> Ditto.
> 
> > +    smp_rmb();
> > 
> >      /*
> >       * Nuke start of day info before checking one last time if the CPU
> > 
> 
> -- 
> Julien Grall
> 

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