From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] CPUIDLE: shorten hpet spin_lock holding time Date: Wed, 21 Apr 2010 09:09:59 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Wei, Gang" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 20/04/2010 17:05, "Wei, Gang" wrote: > Resend. > > CPUIDLE: shorten hpet spin_lock holding time > > Try to reduce spin_lock overhead for deep C state entry/exit. This will > benefit systems with a lot of cpus which need the hpet broadcast to wakeup > from deep C state. > > Signed-off-by: Wei Gang It fixes the unsafe accesses to timer_deadline_{start,end} but I still think this optimisation is misguided and also unsafe. There is nothing to stop new CPUs being added to ch->cpumask after you start scanning ch->cpumask. For example, some new CPU which has a timer_deadline_end greater than ch->next_event, so it does not reprogram the HPET. But handle_hpet_broadcast is already mid-scan and misses this new CPU, so it does not reprogram the HPET either. Hence no timer fires for the new CPU and it misses its deadline. Really I think a better approach than something like this patch would be to better advertise the timer_slop=xxx Xen boot parameter for power-saving scenarios. I wonder what your numbers look like if you re-run your benchmark with (say) timer_slop=10000000 (i.e., 10ms slop) on the Xen command line? -- Keir