CPUIDLE: Write to ARB_DISABLE conditionally to reduce some idle overheads. Signed-off-by: Wei Gang <gang.wei@intel.com> diff -r 48ab8d09f41e xen/arch/x86/acpi/cpu_idle.c --- a/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 15:38:37 2008 +0800 +++ b/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 16:03:09 2008 +0800 @@ -455,8 +455,9 @@ static void acpi_processor_idle(void) if ( power->flags.bm_check && power->flags.bm_control ) { /* Enable bus master arbitration */ + if ( atomic_read(&c3_cpu_count) == num_online_cpus() ) + acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); atomic_dec(&c3_cpu_count); - acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); } /* Re-enable interrupts */ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Sep-03 08:40 UTC
Re: [Xen-devel] [PATCH 3/3] Disable ARB_DIS conditionally
I''m not sure about how safe it would be to have ARB_DISABLE set to 1 while CPUs are not idling. This patch has race that could allow that to happen: Two CPUs stop idling, one sets ARB_DISABLE=0 but before it decrements c3_cpu_count the other does so, then re-enters the idle function, re-increments c3_cpu_count, and sets ARB_DISABLE=1. We now have at least one CPU running normally yet ARB_DISABLE=1. If that race is bad then either we have to leave as is, or synchronise on a spinlock. -- Keir On 3/9/08 02:55, "Wei, Gang" <gang.wei@intel.com> wrote:> CPUIDLE: Write to ARB_DISABLE conditionally to reduce some idle overheads. > > Signed-off-by: Wei Gang <gang.wei@intel.com> > > diff -r 48ab8d09f41e xen/arch/x86/acpi/cpu_idle.c > --- a/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 15:38:37 2008 +0800 > +++ b/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 16:03:09 2008 +0800 > @@ -455,8 +455,9 @@ static void acpi_processor_idle(void) > if ( power->flags.bm_check && power->flags.bm_control ) > { > /* Enable bus master arbitration */ > + if ( atomic_read(&c3_cpu_count) == num_online_cpus() ) > + acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); > atomic_dec(&c3_cpu_count); > - acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); > } > > /* Re-enable interrupts */ > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Thanks for point it out. Of couse we should not let ARB_DISABLE set to 1 while some CPUs are not idling. The real racing case I can imagine is as below: CPU0: -------------judge then clear ARB_DIS----------------------------------dec count CPU1: inc count-------------------------------------judge then set ARB_DIS Sync via spinlock is good suggest, I need to do some testing to make sure spinlock will not bring larger-than-saved overheads. Please hold off this patch. Jimmy On Wednesday, September 03, 2008 4:41 PM, Keir Fraser wrote:> I''m not sure about how safe it would be to have ARB_DISABLE set to 1 while > CPUs are not idling. This patch has race that could allow that to happen: > > Two CPUs stop idling, one sets ARB_DISABLE=0 but before it decrements > c3_cpu_count the other does so, then re-enters the idle function, > re-increments c3_cpu_count, and sets ARB_DISABLE=1. We now have at least one > CPU running normally yet ARB_DISABLE=1. > > If that race is bad then either we have to leave as is, or synchronise on a > spinlock. > > -- Keir > > On 3/9/08 02:55, "Wei, Gang" <gang.wei@intel.com> wrote: > >> CPUIDLE: Write to ARB_DISABLE conditionally to reduce some idle overheads. >> >> Signed-off-by: Wei Gang <gang.wei@intel.com> >> >> diff -r 48ab8d09f41e xen/arch/x86/acpi/cpu_idle.c >> --- a/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 15:38:37 2008 +0800 >> +++ b/xen/arch/x86/acpi/cpu_idle.c Tue Sep 02 16:03:09 2008 +0800 >> @@ -455,8 +455,9 @@ static void acpi_processor_idle(void) >> if ( power->flags.bm_check && power->flags.bm_control ) { >> /* Enable bus master arbitration */ >> + if ( atomic_read(&c3_cpu_count) == num_online_cpus() ) >> + acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); >> atomic_dec(&c3_cpu_count); >> - acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0); } >> >> /* Re-enable interrupts */ >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Sep-03 10:40 UTC
Re: [Xen-devel] [PATCH 3/3] Disable ARB_DIS conditionally
On 3/9/08 11:21, "Wei, Gang" <gang.wei@intel.com> wrote:> CPU0: -------------judge then clear > ARB_DIS----------------------------------dec count > CPU1: inc count-------------------------------------judge then set ARB_DISYes, that''s a more likely race.> Sync via spinlock is good suggest, I need to do some testing to make sure > spinlock will not bring larger-than-saved overheads.Actually I think it''ll be okay. Place the lock next to the c3_cpu_count (so they share a cacheline). Something like: struct { spinlock_t lock; unsigned int count; } c3_cpu_status; Then: * Currently: Exclusive access to one cache line + one LOCK prefix * With lock: Exactly the same (since c3_cpu_count no longer needs to be an atomic_t, and spin_unlock() is also not a LOCKed instruction). But yes, you should test just be to be really sure. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Added spinlock just as you suggested. Resend it. Jimmy On Wednesday, September 03, 2008 6:40 PM, Keir Fraser wrote:> On 3/9/08 11:21, "Wei, Gang" <gang.wei@intel.com> wrote: > >> CPU0: -------------judge then clear >> ARB_DIS----------------------------------dec count >> CPU1: inc count-------------------------------------judge then set ARB_DIS > > Yes, that''s a more likely race. > >> Sync via spinlock is good suggest, I need to do some testing to make sure >> spinlock will not bring larger-than-saved overheads. > > Actually I think it''ll be okay. Place the lock next to the c3_cpu_count (so > they share a cacheline). Something like: > > struct { > spinlock_t lock; > unsigned int count; > } c3_cpu_status; > > Then: > * Currently: Exclusive access to one cache line + one LOCK prefix > * With lock: Exactly the same (since c3_cpu_count no longer needs to be an > atomic_t, and spin_unlock() is also not a LOCKed instruction). > > But yes, you should test just be to be really sure. > > -- Keir_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel