From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH for 4.7 1/4] xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched() Date: Fri, 6 May 2016 15:21:40 +0200 Message-ID: <1462540900.3355.43.camel@citrix.com> References: <146231184906.25631.6550047090421454264.stgit@Solace.fritz.box> <146231199550.25631.15153219462074625034.stgit@Solace.fritz.box> <572A1128.7010609@citrix.com> <1462377503.6981.21.camel@citrix.com> <572A2BDD.4050506@citrix.com> <1462382486.6981.30.camel@citrix.com> <572A32BF.5030404@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6219525890797061743==" Return-path: Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ayfhb-0007fw-Jv for xen-devel@lists.xenproject.org; Fri, 06 May 2016 13:21:55 +0000 In-Reply-To: <572A32BF.5030404@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap , xen-devel@lists.xenproject.org Cc: Wei Liu List-Id: xen-devel@lists.xenproject.org --===============6219525890797061743== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-920K/dk1c+84Scaa2/zt" --=-920K/dk1c+84Scaa2/zt Content-Type: multipart/mixed; boundary="=-/ipTx343DvwmjiZuKO00" --=-/ipTx343DvwmjiZuKO00 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2016-05-04 at 18:34 +0100, George Dunlap wrote: > On 04/05/16 18:21, Dario Faggioli wrote: > >=C2=A0 > > After all, I'm fine with an ASSERT() too, but then I think we > > should > > add one to the same effect to csched_switch_sched() too. > Well an ASSERT() is sort of like a comment, in that if you see > ASSERT(irqs_disabled()), you know there's no need to save irqs > because > they should already disabled.=C2=A0=C2=A0But it has the advantage that os= stest > will be able to "read" it once we get some proper cpupool tests for > osstest. :-) >=20 > If we weren't in the feature freeze, I'd definitely favor adding an > ASSERT to credit1.=C2=A0=C2=A0As it is, I think either way (adding now or > waiting > until the 4.8 development window) should be fine. >=20 Ok, here you go (inline and attached) the patch with ASSERT()-s both in Credit2 and Credit1 (despite the freeze, I think it's the best thing to do, see the changelog). Thanks and Regards, Dario -- commit cbabd44e171d0bd2169f1c7100e69a9e48289980 Author: Dario Faggioli Date:=C2=A0=C2=A0=C2=A0Tue Apr 26 18:56:56 2016 +0200 =C2=A0=C2=A0=C2=A0=C2=A0xen: sched: avoid spuriously re-enabling IRQs in cs= ched2_switch_sched() =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0interrupts are already disabled when calling the ho= ok =C2=A0=C2=A0=C2=A0=C2=A0(from schedule_cpu_switch()), so we must use spin_l= ock() =C2=A0=C2=A0=C2=A0=C2=A0and spin_unlock(). =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0Add an ASSERT(), so we will notice if this code and= its =C2=A0=C2=A0=C2=A0=C2=A0caller get out of sync with respect to disabling in= terrupts =C2=A0=C2=A0=C2=A0=C2=A0(and add one at the same exact occurrence of this p= attern =C2=A0=C2=A0=C2=A0=C2=A0in Credit1 too) =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0Signed-off-by: Dario Faggioli =C2=A0=C2=A0=C2=A0=C2=A0--- =C2=A0=C2=A0=C2=A0=C2=A0Cc: George Dunlap =C2=A0=C2=A0=C2=A0=C2=A0Cc: Wei Liu =C2=A0=C2=A0=C2=A0=C2=A0-- =C2=A0=C2=A0=C2=A0=C2=A0Changes from v1: =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* add the ASSERT(), as requested by George =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* add the ASSERT in Credit1 too =C2=A0=C2=A0=C2=A0=C2=A0-- =C2=A0=C2=A0=C2=A0=C2=A0For Wei: =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- the Credit2 spin_lock_irq()-->spin_lock() c= hange needs =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0to go in, as it fixes a bug; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- adding the ASSERT was requested during revi= ew; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- adding the ASSERT in Credit1 is not strictl= y necessary, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0but imptoves code quality and con= sistency at zero cost =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0and risk, so I think we should ju= st go for it now, instead =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0of waitign for 4.8 (it's basicall= y like I'm adding a =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0comment!). diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index db4d42a..a38a63d 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -615,6 +615,7 @@ csched_switch_sched(struct scheduler *new_ops, unsigned= int cpu, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* schedule_cpu_switch()). It actually m= ay or may not be the 'right' =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* one for this cpu, but that is ok for = preventing races. =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ +=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!local_irq_is_enabled()); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock(&prv->lock); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0init_pdata(prv, pdata, cpu); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock(&prv->lock); diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index f3b62ac..f95e509 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -2238,7 +2238,8 @@ csched2_switch_sched(struct scheduler *new_ops, unsig= ned int cpu, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* And owning exactly that one (the lock= of the old scheduler of this =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* cpu) is what is necessary to prevent = races. =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ -=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irq(&prv->lock); +=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!local_irq_is_enabled()); +=C2=A0=C2=A0=C2=A0=C2=A0spin_lock(&prv->lock); =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0idle_vcpu[cpu]->sched_priv =3D vdata; =C2=A0 @@ -2263,7 +2264,7 @@ csched2_switch_sched(struct scheduler *new_ops, unsig= ned int cpu, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0smp_mb(); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0per_cpu(schedule_data, cpu).schedule_lock =3D= &prv->rqd[rqi].lock; =C2=A0 -=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irq(&prv->lock); +=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock(&prv->lock); =C2=A0} =C2=A0 =C2=A0static void --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-/ipTx343DvwmjiZuKO00 Content-Disposition: attachment; filename="xen-sched-avoid-spuriously-reenable-irqs.patch" Content-Transfer-Encoding: base64 Content-Type: text/x-patch; name="xen-sched-avoid-spuriously-reenable-irqs.patch"; charset="UTF-8" Y29tbWl0IGNiYWJkNDRlMTcxZDBiZDIxNjlmMWM3MTAwZTY5YTllNDgyODk5ODAKQXV0aG9yOiBE YXJpbyBGYWdnaW9saSA8ZGFyaW8uZmFnZ2lvbGlAY2l0cml4LmNvbT4KRGF0ZTogICBUdWUgQXBy IDI2IDE4OjU2OjU2IDIwMTYgKzAyMDAKCiAgICB4ZW46IHNjaGVkOiBhdm9pZCBzcHVyaW91c2x5 IHJlLWVuYWJsaW5nIElSUXMgaW4gY3NjaGVkMl9zd2l0Y2hfc2NoZWQoKQogICAgCiAgICBpbnRl cnJ1cHRzIGFyZSBhbHJlYWR5IGRpc2FibGVkIHdoZW4gY2FsbGluZyB0aGUgaG9vawogICAgKGZy b20gc2NoZWR1bGVfY3B1X3N3aXRjaCgpKSwgc28gd2UgbXVzdCB1c2Ugc3Bpbl9sb2NrKCkKICAg IGFuZCBzcGluX3VubG9jaygpLgogICAgCiAgICBBZGQgYW4gQVNTRVJUKCksIHNvIHdlIHdpbGwg bm90aWNlIGlmIHRoaXMgY29kZSBhbmQgaXRzCiAgICBjYWxsZXIgZ2V0IG91dCBvZiBzeW5jIHdp dGggcmVzcGVjdCB0byBkaXNhYmxpbmcgaW50ZXJydXB0cwogICAgKGFuZCBhZGQgb25lIGF0IHRo ZSBzYW1lIGV4YWN0IG9jY3VycmVuY2Ugb2YgdGhpcyBwYXR0ZXJuCiAgICBpbiBDcmVkaXQxIHRv bykKICAgIAogICAgU2lnbmVkLW9mZi1ieTogRGFyaW8gRmFnZ2lvbGkgPGRhcmlvLmZhZ2dpb2xp QGNpdHJpeC5jb20+CiAgICAtLS0KICAgIENjOiBHZW9yZ2UgRHVubGFwIDxnZW9yZ2UuZHVubGFw QGNpdHJpeC5jb20+CiAgICBDYzogV2VpIExpdSA8d2VpLmxpdTJAY2l0cml4LmNvbT4KICAgIC0t CiAgICBDaGFuZ2VzIGZyb20gdjE6CiAgICAgKiBhZGQgdGhlIEFTU0VSVCgpLCBhcyByZXF1ZXN0 ZWQgYnkgR2VvcmdlCiAgICAgKiBhZGQgdGhlIEFTU0VSVCBpbiBDcmVkaXQxIHRvbwogICAgLS0K ICAgIEZvciBXZWk6CiAgICAgLSB0aGUgQ3JlZGl0MiBzcGluX2xvY2tfaXJxKCktLT5zcGluX2xv Y2soKSBjaGFuZ2UgbmVlZHMKICAgICAgIHRvIGdvIGluLCBhcyBpdCBmaXhlcyBhIGJ1ZzsKICAg ICAtIGFkZGluZyB0aGUgQVNTRVJUIHdhcyByZXF1ZXN0ZWQgZHVyaW5nIHJldmlldzsKICAgICAt IGFkZGluZyB0aGUgQVNTRVJUIGluIENyZWRpdDEgaXMgbm90IHN0cmljdGx5IG5lY2Vzc2FyeSwK ICAgICAgIGJ1dCBpbXB0b3ZlcyBjb2RlIHF1YWxpdHkgYW5kIGNvbnNpc3RlbmN5IGF0IHplcm8g Y29zdAogICAgICAgYW5kIHJpc2ssIHNvIEkgdGhpbmsgd2Ugc2hvdWxkIGp1c3QgZ28gZm9yIGl0 IG5vdywgaW5zdGVhZAogICAgICAgb2Ygd2FpdGlnbiBmb3IgNC44IChpdCdzIGJhc2ljYWxseSBs aWtlIEknbSBhZGRpbmcgYQogICAgICAgY29tbWVudCEpLgoKZGlmZiAtLWdpdCBhL3hlbi9jb21t b24vc2NoZWRfY3JlZGl0LmMgYi94ZW4vY29tbW9uL3NjaGVkX2NyZWRpdC5jCmluZGV4IGRiNGQ0 MmEuLmEzOGE2M2QgMTAwNjQ0Ci0tLSBhL3hlbi9jb21tb24vc2NoZWRfY3JlZGl0LmMKKysrIGIv eGVuL2NvbW1vbi9zY2hlZF9jcmVkaXQuYwpAQCAtNjE1LDYgKzYxNSw3IEBAIGNzY2hlZF9zd2l0 Y2hfc2NoZWQoc3RydWN0IHNjaGVkdWxlciAqbmV3X29wcywgdW5zaWduZWQgaW50IGNwdSwKICAg ICAgKiBzY2hlZHVsZV9jcHVfc3dpdGNoKCkpLiBJdCBhY3R1YWxseSBtYXkgb3IgbWF5IG5vdCBi ZSB0aGUgJ3JpZ2h0JwogICAgICAqIG9uZSBmb3IgdGhpcyBjcHUsIGJ1dCB0aGF0IGlzIG9rIGZv ciBwcmV2ZW50aW5nIHJhY2VzLgogICAgICAqLworICAgIEFTU0VSVCghbG9jYWxfaXJxX2lzX2Vu YWJsZWQoKSk7CiAgICAgc3Bpbl9sb2NrKCZwcnYtPmxvY2spOwogICAgIGluaXRfcGRhdGEocHJ2 LCBwZGF0YSwgY3B1KTsKICAgICBzcGluX3VubG9jaygmcHJ2LT5sb2NrKTsKZGlmZiAtLWdpdCBh L3hlbi9jb21tb24vc2NoZWRfY3JlZGl0Mi5jIGIveGVuL2NvbW1vbi9zY2hlZF9jcmVkaXQyLmMK aW5kZXggZjNiNjJhYy4uZjk1ZTUwOSAxMDA2NDQKLS0tIGEveGVuL2NvbW1vbi9zY2hlZF9jcmVk aXQyLmMKKysrIGIveGVuL2NvbW1vbi9zY2hlZF9jcmVkaXQyLmMKQEAgLTIyMzgsNyArMjIzOCw4 IEBAIGNzY2hlZDJfc3dpdGNoX3NjaGVkKHN0cnVjdCBzY2hlZHVsZXIgKm5ld19vcHMsIHVuc2ln bmVkIGludCBjcHUsCiAgICAgICogQW5kIG93bmluZyBleGFjdGx5IHRoYXQgb25lICh0aGUgbG9j ayBvZiB0aGUgb2xkIHNjaGVkdWxlciBvZiB0aGlzCiAgICAgICogY3B1KSBpcyB3aGF0IGlzIG5l Y2Vzc2FyeSB0byBwcmV2ZW50IHJhY2VzLgogICAgICAqLwotICAgIHNwaW5fbG9ja19pcnEoJnBy di0+bG9jayk7CisgICAgQVNTRVJUKCFsb2NhbF9pcnFfaXNfZW5hYmxlZCgpKTsKKyAgICBzcGlu X2xvY2soJnBydi0+bG9jayk7CiAKICAgICBpZGxlX3ZjcHVbY3B1XS0+c2NoZWRfcHJpdiA9IHZk YXRhOwogCkBAIC0yMjYzLDcgKzIyNjQsNyBAQCBjc2NoZWQyX3N3aXRjaF9zY2hlZChzdHJ1Y3Qg c2NoZWR1bGVyICpuZXdfb3BzLCB1bnNpZ25lZCBpbnQgY3B1LAogICAgIHNtcF9tYigpOwogICAg IHBlcl9jcHUoc2NoZWR1bGVfZGF0YSwgY3B1KS5zY2hlZHVsZV9sb2NrID0gJnBydi0+cnFkW3Jx aV0ubG9jazsKIAotICAgIHNwaW5fdW5sb2NrX2lycSgmcHJ2LT5sb2NrKTsKKyAgICBzcGluX3Vu bG9jaygmcHJ2LT5sb2NrKTsKIH0KIAogc3RhdGljIHZvaWQK --=-/ipTx343DvwmjiZuKO00-- --=-920K/dk1c+84Scaa2/zt 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 iQIcBAABCAAGBQJXLJpkAAoJEBZCeImluHPu+0IP/13MQGre9QMN6r4OQEH848FH qfZs0EP9Gg8oTAWmM1eFEGAo3lstLAlX6e0njUwYxeEXUXGRgi5YqTI7guio4tUl 79L1AqCU9CqWrSApuN6a8GxvyQ3YeRRupPWbAGQOVjA4fOfkYbrpG2ii2aa1D1aX 1aTb+oe/eHgLsj2XGNcJeTblt2B1ZzH6G961Ge3Ayt+yOXW3OXrEYr4orR9ak1dx eEmc1QvtlL8rmNqN2gNZCm5/2OldkMpZuVoCl0IFf3WI2uRQNGD1Jhqr1Shu9yQx NtUSYHzZyzzZUExyl/JD+VmbwnS/GcUJpBofuuBiJlP7BGgZti5tQsgE9PIjy2BJ /kGhNeaX2hH/xDQxmIOPouIOgtjd4aftFXb3wvyK1mz+Uw/7xKNESwklJmUOz5ZD cEOJDPLT+AZNEy/WaH0t6ppJZ0+8yirCtSfW+JJ5ulLuzajhl2PErelxrVNw3qcl JSNT2/T/o2A43lePaf91TPokT7ZNhi2K31iZOGBoQVrLCe/beDIN0v3OLNyMj5y4 dlTU49ymMegRyiscB8eT6MIPC2NuIWg8cgdRYePOGwzmQOq/8/6gRMXkTtS00vAo F6QbzPRWokURvqvOpWqyPd5ClHeG1BQLV+J1qQuGJF+uVNY2l8QWbN2br9oEphX1 3eyK39rlFNcLbBaZ4v4W =R0n8 -----END PGP SIGNATURE----- --=-920K/dk1c+84Scaa2/zt-- --===============6219525890797061743== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============6219525890797061743==--