From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 06/15] xen: trace IRQ enabling/disabling Date: Fri, 2 Jun 2017 01:42:59 +0200 Message-ID: <1496360579.10189.13.camel@citrix.com> References: <149633614204.12814.14390287626133023934.stgit@Solace.fritz.box> <149633845700.12814.7130992212550379105.stgit@Solace.fritz.box> <8f55c37d-3cd0-af58-4217-2b0b73886234@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5585362183201474664==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dGZkH-0001JX-J1 for xen-devel@lists.xenproject.org; Thu, 01 Jun 2017 23:43:13 +0000 In-Reply-To: <8f55c37d-3cd0-af58-4217-2b0b73886234@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Andrew Cooper , xen-devel@lists.xenproject.org Cc: George Dunlap , Julien Grall , Stefano Stabellini , Jennifer Herbert , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============5585362183201474664== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-cBzhKGf1nL8AvYVEXZA/" --=-cBzhKGf1nL8AvYVEXZA/ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2017-06-01 at 20:08 +0100, Andrew Cooper wrote: > On 01/06/17 18:34, Dario Faggioli wrote: > > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c > > index 2a06406..33b903e 100644 > > --- a/xen/common/spinlock.c > > +++ b/xen/common/spinlock.c > > @@ -150,7 +150,9 @@ void _spin_lock(spinlock_t *lock) > > =C2=A0void _spin_lock_irq(spinlock_t *lock) > > =C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(local_irq_is_enabled()); > > -=C2=A0=C2=A0=C2=A0=C2=A0local_irq_disable(); > > +=C2=A0=C2=A0=C2=A0=C2=A0_local_irq_disable(); > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( unlikely(tb_init_done) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_irq_disable_ret(= ); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0_spin_lock(lock); > > =C2=A0} >=20 > Is it sensible to do this way around? >=20 Well, I guess either variant has up and down sides, and it looks to me that there is no way to measure this, without interfering with the behavior of the thing being measured ("Once upon a time, there was a cat, who lived in a box. His name was Schr=C3=B6dinger..." :-D :-D :-D) > By writing the trace record while interrupts are disabled, you do > prevent nesting in the general case (but not in NMIs/MCEs or the > irqsave() variants),=20 > Forgive the ignorance, what's special about NMIs/MCAs that is relevant for this? About _irqsave(), I'm also not sure what you mean, as _irqsave() is already done differently than this. > but you increase the critical region while > interrupts are disabled. >=20 I know. > Alternatively, writing the trace record outside of the critical > region > would reduce the size of the region.=C2=A0=C2=A0Interpretation logic alre= ady > needs > to cope with nesting, so is one extra level of nesting in this corner > case a problem? >=20 TBH, I was indeed trying to offer to the component that will be looking at and interpreting the data, the as clear as possible view on when IRQs are _actually_ disabled and enabled. As in, if nesting occurs, only the event corresponding to: - the first local_irq_disable() - the last local_irq_enable() I.e., to not require that it (the interpretation logic) does understand nesting. But more than this, what I was concerned about was the accuracy of the reporting itself. In fact, if I do as you suggest, I can be interrupted (as interrupts are still on) after having called trace_irq_disable(). That means the report will show higher IRQ disabled time period, for this instance, than what the reality is. And the same is true on the enabling side, when I call trace_irq_enable() _before_ re-enabling interrupts, which has the same bad effect on the length of IRQ disabled section. Maybe, considering that anything that will interrupt me in these cases, are other interrupts, that will most likely disable interrupts themselves, I should not worry too much about this... What do you think? > Does the logic cope with the fact that interrupt gates automatically > disable interrupts? >=20 Ah, right. No, it does not. I probably should mention this in the changelog. Any ideas of how to deal with that? If yes, I'm more than happy to fix this... Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-cBzhKGf1nL8AvYVEXZA/ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJZMKaDAAoJEBZCeImluHPuc7oQAOaGoKxADAbaPzJL7mlOt+t+ lYaRm7nIu3cACICtmYZPBZoOZK5v+dOuMtQht3gNtaLORIvLhghuhgeJabBWSuZY wDYXk0+/QLF8yU/XrCjuFGDsChqbkxns1aM1ETefKu2AKI7RV9wEu9xwlzbDJoy/ FIvgSgVTu44rXv93GigXZFETzUBkeXfdfqAd0dgvpNTaoZbAju/mnJjoIXNSX5cr /1/+11PrdxTEhBzNjsQM/88BqtN5+7oVSext6aTrtWEK5xi22m9nwucAYph8ZU9z JZJ5KWCYPQUmgsRTcYxlKi4iEAa98f55FJC4OC2jZIGGPzBlSciLSUsp9TlmKgip 6xl9FwmfJ4h7jXVwTs6ncPgP0d6ntIzhbCKWymo9jHAWJ997jEJuvGZo2ViIaTA6 BMvabsGfn45uyKOwFrgO/gNVcu2GRDNTOBMx+ubbi+IiBTrGafFuw+bA0xA29ctn 3j69/1AliEwinjJoHyIr1DnwzKvok0QV2h1WovhNdFTNTQNO1QilQRRqOa7f8SDe es4RGdZN4n8LUPwepQAzwAJA8FZI6hOuz7ONDBlXWx3ZlYCemTyTWN08kS9YRzod 7LIprzTott4niKfVS5/FyHVXXXJSNssDV/SX6zIpcY+7zaKYsPrRS93PdV/UUcf2 8IcHKqBddhRtTvSP/JY3 =dfEc -----END PGP SIGNATURE----- --=-cBzhKGf1nL8AvYVEXZA/-- --===============5585362183201474664== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============5585362183201474664==--