[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/4] xen/x86: Drop erronious barriers
>>> 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(). Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |