From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] CPUIDLE: shorten hpet spin_lock holding time Date: Tue, 20 Apr 2010 15:21:44 +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 15:04, "Wei, Gang" wrote: > On Tuesday, 2010-4-20 8:49 PM, Keir Fraser wrote: >> Is this a measurable win? The newer locking looks like it could be >> dodgy on 32-bit Xen: the 64-bit reads of timer_deadline_{start,end} >> will be non-atomic and unsynchronised so you can read garbage. Even >> on 64-bit Xen you can read stale values. I'll be surprised if you got >> a performance win from chopping up critical regions in individual >> functions like that anyway. > > First of all, this is a measurable power win for cpu overcommitment idle case > (vcpu:pcpu > 2:1, pcpu >= 32, guests are non-tickless kernels). So lots of short sleep periods, and possibly only a very few HPET channels to share? How prevalent is always-running APIC timer now, and is that going to be supported in future processors? > As to the non-atomic access to timer_deadline_{start,end}, it should already > be there before this patch. It is not protected by the hpet lock. Shall we add > rw_lock for each timer_deadline_{start,end}? This can be done separately. The bug isn't previously there, since the fields will not be read unless the cpu is in ch->cpumask, which (was) protected by ch->lock. That was sufficient since a CPU would not modify timer_deadline_{start,end} between hpet_broadcast_enter and hpet_broadcast_exit. After your patch, handle_hpet_broadcast is no longer fully synchronised against those functions. -- Keir