Dave Winchell
2008-Oct-23 18:23 UTC
[Xen-devel][PATCH] add memory barriers to IPI related code
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2008-Oct-23 18:31 UTC
Re: [Xen-devel][PATCH] add memory barriers to IPI related code
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@virtualiron.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Dave Winchell
2008-Oct-27 14:28 UTC
Re: [Xen-devel][PATCH] add memory barriers to IPI related code
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@virtualiron.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel