From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [Patch] don't spin with irq disabled Date: Fri, 27 Mar 2009 07:41:03 +0000 Message-ID: <49CC911F.76E4.0078.0@novell.com> References: <49CB441E.9030006@fujitsu-siemens.com> <49CB81C2.76E4.0078.0@novell.com> <49CB76B5.3040902@fujitsu-siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <49CB76B5.3040902@fujitsu-siemens.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Juergen Gross Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> Juergen Gross 26.03.09 13:36 >>> >Jan Beulich wrote: >>>>> Juergen Gross 26.03.09 10:00 >>> >>> Attached patch reduces interrupt latency in lock handling. >>> spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then = tried to >>> get the lock. If the lock was already held, waiting for the lock was = done with >>> IRQs still off. >>> The patch reenables IRQs during the wait loop. >>=20 >> This is wrong - you must not enable interrupts if they weren't enabled = upon >> entry to these two functions. > >spin_lock_irq disables always IRQs. spin_unlock_irq enables always IRQs. = They >are always used in pairs, so IRQs should always be enabled on entry of >spin_lock_irq. No, I wouldn't suggest making an assumption like this - some code could = allow interrupts to be disabled when acquiring the lock, but intentionally = enabling them when releasing it. (Personally, I think there shouldn't be any users = of this function pair in the first place, as I don't think forcibly enabling = interrupts is a correct thing to do in all but *very* few cases.) >I'm not enabling IRQs unconditionally in spin_lock_irqsave, of course, = but use >the flags value saved before... Oh, sorry, I blindly implied the second function to use the same methods = as the first one. And really I'd think it's cheaper to use local_irq_disable()= here (but of course retain local_irq_restore()). Btw., why do you think the issue is more important to address in Xen than in Linux (where not to long ago the opposite move happened, since re- enabling interrupts with ticket locks is much trickier and hence wasn't considered worthwhile afair. Jan