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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 5 Dec 2016, Jan Beulich wrote:
> >>> On 05.12.16 at 14:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 05/12/16 13:50, Jan Beulich wrote:
> >>>>> On 05.12.16 at 14:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>> On 05/12/16 12:28, Jan Beulich wrote:
> >>>>>>> On 05.12.16 at 12:25, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>>> On 05/12/16 11:18, Jan Beulich wrote:
> >>>>>>>>> On 05.12.16 at 11:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>>>>> --- a/xen/arch/x86/smpboot.c
> >>>>>>> +++ b/xen/arch/x86/smpboot.c
> >>>>>>> @@ -346,7 +346,6 @@ void start_secondary(void *unused)
> >>>>>>>      spin_debug_enable();
> >>>>>>>      set_cpu_sibling_map(cpu);
> >>>>>>>      notify_cpu_starting(cpu);
> >>>>>>> -    wmb();
> >>>>>>>  
> >>>>>>>      /*
> >>>>>>>       * We need to hold vector_lock so there the set of online cpus
> >>>>>> Hmm, this one is indeed redundant with the lock_vector_lock()
> >>>>>> following right below, but if that lock wasn't there, I think it
> >>>>>> would be needed to order set_cpu_sibling_map() and the
> >>>>>> setting of the bit in the online map. So I think it would better
> >>>>>> stay (but be relaxed to smb_wmb()).
> >>>>> Why?  It doesn't relate to an smp_rmb() on any other CPU, and is
> >>>>> therefore wrong to use.
> >>>> I think it does, just not with one that's spelled out as smp_rmb().
> >>>> Instead __cpu_up() spins on !cpu_online(), using cpu_relax() as
> >>>> a de-facto equivalent of smp_rmb().
> >>> __cpu_up() is ordered with the cpumask_set_cpu(cpu, &cpu_online_map)
> >>> between the two context hunks.
> >> Exactly - so here we need the write side to that
> > 
> > No, we don't.
> > 
> > cpumask_set_cpu(cpu, &cpu_online_map) is a write operation, so orders
> > properly on x86.  C's ordering properties ensure that the adjacent
> > function calls happen in program order.
> 
> Well, that then again falls into the category of barriers which
> would be needed in arch-independent code, but can be omitted
> in x86-specific sources. I think we should separate both classes
> when relaxing/eliminating them.

Yes. It would be nice to keep a barrier, one that #define to nothing if
it's unneeded, so that if somebody else on a different arch (*cough*
*cough*) ends up copying the code, it will know what to do.

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