Re: [Xen-devel][PATCH] add memory barriers to IPI related code

[Dave Winchell <dwinchell@xxxxxxxxxxxxxxx>]

After discussions with Kevin and other folks at Intel, and some 
experimentation
here, I am convinced that this patch is not needed. The reason for my hangs
was that I had the initial cs for x2apic, but not the second one, which 
had the
crucial mb() in the send_IPI_mask_x2apic. The mb()s I added to fix the
problem were more than the minimum required. Indeed it appears that taking
an interrupt implies a mb() on ia32. Thanks to everyone who helped me 
with this,
especially Kevin Tian.

-Dave


Keir Fraser wrote:

> I'd like some advice from Intel on the need for all these barriers. I'm
> pretty sure that interrupts are supposed to imply memory barriers on the x86
> architecture. It's not beyond the realm of possibility that you're running
> on a buggy SDP.
>
> You certainly should not need barriers on the sending side, since (apart
> from x2apic, which already has an explicit mb()) the send is performed by a
> memory write, and stores are guaranteed to occur in-order on x86
> architecture. So e.g., call_data must be visible before the IPI occurs in
> smp_call_function().
>
> And on the receiving side, it's hard to see how a read would appear to occur
> before the interrupt which transfers control to the IPI handler. Unless
> there's really weird cache synchronisation magic going on.
>
> -- Keir
>
> On 23/10/08 19:23, "Dave Winchell" <dwinchell@xxxxxxxxxxxxxxx> wrote:
>
>  
>
> > Hi Keir,
> >
> > We have been seeing some hangs in call function and
> > tlb shootdown. In our case the machine was an Intel Tylersburg.
> >
> > One failure mode is this early return in smp_call_function_interrupt:
> >
> >     if ( !cpu_isset(smp_processor_id(), call_data->selected) )
> >         return;
> >
> > Without a mb(), the target processor takes this path as it doesn't yet
> > see the update
> > to call_data->selected from the initiating processor. Then the initiator
> > waits for ever
> > to get an acknowledgment from this processor.
> >
> > On the initiating side, I added the mb()'s to the send_IPI functions,
> > x2apic already
> > having one.
> >
> > On the target side, I added the mb()'s in the individual target functions.
> >
> > Regards,
> > Dave
> >
> > diff -r 4129f0f2f2ba xen/arch/x86/smp.c
> > --- a/xen/arch/x86/smp.c Fri Oct 17 14:15:37 2008 +0100
> > +++ b/xen/arch/x86/smp.c Mon Oct 20 10:53:24 2008 -0400
> > @@ -93,6 +93,8 @@ void send_IPI_mask_flat(cpumask_t cpumas
> >     /* An IPI with no target generates a send accept error from P5/P6 APICs.
> > */
> >     WARN_ON(mask == 0);
> >
> > +    mb();
> > +
> >     local_irq_save(flags);
> >
> >     /*
> > @@ -123,6 +125,8 @@ void send_IPI_mask_phys(cpumask_t mask,
> > {
> >     unsigned long cfg, flags;
> >     unsigned int query_cpu;
> > +
> > +    mb();
> >
> >     local_irq_save(flags);
> >
> > @@ -160,6 +164,7 @@ static unsigned int flush_flags;
> >
> > fastcall void smp_invalidate_interrupt(void)
> > {
> > +    mb();
> >     ack_APIC_irq();
> >     perfc_incr(ipis);
> >     irq_enter();
> > @@ -337,17 +342,22 @@ void smp_send_stop(void)
> >
> > fastcall void smp_event_check_interrupt(struct cpu_user_regs *regs)
> > {
> > +    mb();
> >     ack_APIC_irq();
> >     perfc_incr(ipis);
> > }
> >
> > fastcall void smp_call_function_interrupt(struct cpu_user_regs *regs)
> > {
> > -    void (*func)(void *info) = call_data->func;
> > -    void *info = call_data->info;
> > -
> > +    void (*func)(void *info);
> > +    void *info;
> > +
> > +    mb();
> >     ack_APIC_irq();
> >     perfc_incr(ipis);
> > +
> > +    func = call_data->func;
> > +    info = call_data->info;
> >
> >     if ( !cpu_isset(smp_processor_id(), call_data->selected) )
> >         return;
> >    
>
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel