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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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/crash.c
>>>> +++ b/xen/arch/x86/crash.c
>>>> @@ -146,9 +146,6 @@ static void nmi_shootdown_cpus(void)
>>>>      write_atomic((unsigned long *)__va(__pa(&exception_table[TRAP_nmi])),
>>>>                   (unsigned long)&do_nmi_crash);
>>>>  
>>>> -    /* Ensure the new callback function is set before sending out the 
>>>> NMI. */
>>>> -    wmb();
>>>> -
>>>>      smp_send_nmi_allbutself();
>>> I don't think I agree with this change - we certainly want to make
>>> sure the APIC write happens only after after the exception vector
>>> adjustment became visible, namely when in x2APIC mode (where
>>> the respective WRMSRs are not serializing).
>> This wmb() is already only a barrier() (Fixed in the final patch)
> Good point.
>
>> Even if it weren't, wrmsr doesn't interact with sfence, so the barrier
>> would still be pointless.
> Are you sure there's absolutely nothing in replacement for the lack
> of serialization?
>
> That said, we're still having enough of a barrier left anyway, due to
> the ones in send_IPI_mask_x2apic_{phys,cluster}(), and due to the
> LAPIC mapping being UC. So yes, I'm fine with dropping it here then.
>
>>>> --- 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.

I still don't see any purpose or need for there to be any barriers here
at all, not even compiler barriers.

~Andrew

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