From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: Re: [PATCH] CPUIDLE: shorten hpet spin_lock holding time Date: Wed, 21 Apr 2010 11:03:08 +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 21/04/2010 10:52, "Keir Fraser" wrote: > Okay, one concern I still have is over possible races around > cpuidle_wakeup_mwait(). It makes use of a cpumask cpuidle_mwait_flags, > avoiding an IPI to cpus in the mask. However, there is nothing to stop the > CPU having cleared itself from that cpumask before cpuidle does the write to > softirq_pending. In that case, even assuming the CPU is now non-idle and so > wakeup is spurious, a subsequent attempt to raise_softirq(TIMER_SOFTIRQ) > will incorrectly not IPI because the flag is already set in softirq_pending? Oh, another one, which can **even occur without your patch**: CPU A adds itself to cpuidle_mwait_flags while cpuidle_wakeup_mwait() is running. That function doesn't see CPU A in its first read of the cpumask so it does not set TIMER_SOFTIRQ for CPU A. But it then *does* see CPU A in its final read of the cpumask, and hence clears A from the caller's mask. Hence the caller does not pass CPU A to cpumask_raise_softirq(). Hence CPU A is erroneously not woken. Isn't the synchronisation around those mwait/monitor functions just inherently broken, even without your patch, and your patch just makes it worse? :-) -- Keir