From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch() Date: Wed, 21 Oct 2015 19:10:29 +0200 Message-ID: <1445447429.3009.209.camel@citrix.com> References: <20151014151805.28642.63981.stgit@Solace.station> <20151014155416.28642.76465.stgit@Solace.station> <561F630B.5080401@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4158183802285865648==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZowuK-00026I-0Z for xen-devel@lists.xenproject.org; Wed, 21 Oct 2015 17:10:36 +0000 In-Reply-To: <561F630B.5080401@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Juergen Gross , xen-devel@lists.xenproject.org Cc: George Dunlap , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============4158183802285865648== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-IIqgnRvmB41h2mCwRkbt" --=-IIqgnRvmB41h2mCwRkbt Content-Type: multipart/mixed; boundary="=-fiP2hMyb249kJUsRlMqH" --=-fiP2hMyb249kJUsRlMqH Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2015-10-15 at 10:25 +0200, Juergen Gross wrote: > Maybe it would be a good idea to move setting of per_cpu(cpupool, > cpu) > into schedule_cpu_switch(). Originally I didn't do that to avoid > spreading too much cpupool related actions outside of cpupool.c. But > with those ASSERT()s added hiding that action will cause more > confusion > than having the modification of per_cpu(cpupool, cpu) here. >=20 Coming back to this. When reworking the series, I tried to move 'per_cpu(cpupool, cpu)=3Dc' in schedule_cpu_switch() and, about this part: > When doing the code movement the current behaviour regarding sequence > of changes must be kept, of course. So when adding the cpu to a pool > the cpupool information must be set _before_ taking the scheduler > lock, > while when removing this must happen after release of the lock. >=20 I don't think I see why I can't do as in the attached patch (i.e., just update the cpupool per-cpu variable at the bottom of schedule_cpu_switch() ). I don't see anything in the various schedulers' code that relies on that variable, except one thing in sched_credit.c, which looks safe to me. And indeed I think that even any future potential code being added there, should either not depend on it, or do it "safely". Also, all the operations done in schedule_cpu_switch() itself, depend either on per_cpu(scheduler) or on per_cpu(schedule_data) being updated properly, rather than on per_cpu(cpupool) (including the locking that you are mentioning above). What am I missing? Regards, Dario --- diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c index e79850b..8e7b723 100644 --- a/xen/common/cpupool.c +++ b/xen/common/cpupool.c @@ -261,19 +261,13 @@ int cpupool_move_domain(struct domain *d, struct cpup= ool *c) static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu) { int ret; - struct cpupool *old; struct domain *d; =20 if ( (cpupool_moving_cpu =3D=3D cpu) && (c !=3D cpupool_cpu_moving) ) return -EBUSY; - old =3D per_cpu(cpupool, cpu); - per_cpu(cpupool, cpu) =3D c; ret =3D schedule_cpu_switch(cpu, c); if ( ret ) - { - per_cpu(cpupool, cpu) =3D old; return ret; - } =20 cpumask_clear_cpu(cpu, &cpupool_free_cpus); if (cpupool_moving_cpu =3D=3D cpu) @@ -326,7 +320,6 @@ static long cpupool_unassign_cpu_helper(void *info) cpumask_clear_cpu(cpu, &cpupool_free_cpus); goto out; } - per_cpu(cpupool, cpu) =3D NULL; cpupool_moving_cpu =3D -1; cpupool_put(cpupool_cpu_moving); cpupool_cpu_moving =3D NULL; diff --git a/xen/common/schedule.c b/xen/common/schedule.c index d7e2d98..9072540 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1486,6 +1486,17 @@ void __init scheduler_init(void) BUG(); } =20 +/* + * Move a pCPU outside of the influence of the scheduler of its current + * cpupool, or subject it to the scheduler of a new cpupool. + * + * For the pCPUs that are removed from their cpupool, their scheduler beco= mes + * &ops (the default scheduler, selected at boot, which also services the + * default cpupool). However, as these pCPUs are not really part of any po= ol, + * there won't be any scheduling event on them, not even from the default + * scheduler. Basically, they will just sit idle until they are explicitly + * added back to a cpupool. + */ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c) { unsigned long flags; @@ -1494,9 +1505,24 @@ int schedule_cpu_switch(unsigned int cpu, struct cpu= pool *c) void *ppriv, *ppriv_old, *vpriv, *vpriv_old; struct scheduler *old_ops =3D per_cpu(scheduler, cpu); struct scheduler *new_ops =3D (c =3D=3D NULL) ? &ops : c->sched; + struct cpupool *old_pool =3D per_cpu(cpupool, cpu); + + /* + * pCPUs only move from a valid cpupool to free (i.e., out of any pool= ), + * or from free to a valid cpupool. In the former case (which happens = when + * c is NULL), we want the CPU to have been marked as free already, as + * well as to not be valid for the source pool any longer, when we get= to + * here. In the latter case (which happens when c is a valid cpupool),= we + * want the CPU to still be marked as free, as well as to not yet be v= alid + * for the destination pool. + */ + ASSERT(c !=3D old_pool && (c !=3D NULL || old_pool !=3D NULL)); + ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus)); + ASSERT((c =3D=3D NULL && !cpumask_test_cpu(cpu, old_pool->cpu_valid)) = || + (c !=3D NULL && !cpumask_test_cpu(cpu, c->cpu_valid))); =20 if ( old_ops =3D=3D new_ops ) - return 0; + goto out; =20 idle =3D idle_vcpu[cpu]; ppriv =3D SCHED_OP(new_ops, alloc_pdata, cpu); @@ -1524,6 +1550,9 @@ int schedule_cpu_switch(unsigned int cpu, struct cpup= ool *c) SCHED_OP(old_ops, free_vdata, vpriv_old); SCHED_OP(old_ops, free_pdata, ppriv_old, cpu); =20 + out: + per_cpu(cpupool, cpu) =3D c; + return 0; } =20 --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-fiP2hMyb249kJUsRlMqH Content-Disposition: attachment; filename="xen-sched-asserts-in-schedule_cpu_switch.patch" Content-Type: text/x-patch; name="xen-sched-asserts-in-schedule_cpu_switch.patch"; charset="UTF-8" Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL3hlbi9jb21tb24vY3B1cG9vbC5jIGIveGVuL2NvbW1vbi9jcHVwb29sLmMK aW5kZXggZTc5ODUwYi4uOGU3YjcyMyAxMDA2NDQKLS0tIGEveGVuL2NvbW1vbi9jcHVwb29sLmMK KysrIGIveGVuL2NvbW1vbi9jcHVwb29sLmMKQEAgLTI2MSwxOSArMjYxLDEzIEBAIGludCBjcHVw b29sX21vdmVfZG9tYWluKHN0cnVjdCBkb21haW4gKmQsIHN0cnVjdCBjcHVwb29sICpjKQogc3Rh dGljIGludCBjcHVwb29sX2Fzc2lnbl9jcHVfbG9ja2VkKHN0cnVjdCBjcHVwb29sICpjLCB1bnNp Z25lZCBpbnQgY3B1KQogewogICAgIGludCByZXQ7Ci0gICAgc3RydWN0IGNwdXBvb2wgKm9sZDsK ICAgICBzdHJ1Y3QgZG9tYWluICpkOwogCiAgICAgaWYgKCAoY3B1cG9vbF9tb3ZpbmdfY3B1ID09 IGNwdSkgJiYgKGMgIT0gY3B1cG9vbF9jcHVfbW92aW5nKSApCiAgICAgICAgIHJldHVybiAtRUJV U1k7Ci0gICAgb2xkID0gcGVyX2NwdShjcHVwb29sLCBjcHUpOwotICAgIHBlcl9jcHUoY3B1cG9v bCwgY3B1KSA9IGM7CiAgICAgcmV0ID0gc2NoZWR1bGVfY3B1X3N3aXRjaChjcHUsIGMpOwogICAg IGlmICggcmV0ICkKLSAgICB7Ci0gICAgICAgIHBlcl9jcHUoY3B1cG9vbCwgY3B1KSA9IG9sZDsK ICAgICAgICAgcmV0dXJuIHJldDsKLSAgICB9CiAKICAgICBjcHVtYXNrX2NsZWFyX2NwdShjcHUs ICZjcHVwb29sX2ZyZWVfY3B1cyk7CiAgICAgaWYgKGNwdXBvb2xfbW92aW5nX2NwdSA9PSBjcHUp CkBAIC0zMjYsNyArMzIwLDYgQEAgc3RhdGljIGxvbmcgY3B1cG9vbF91bmFzc2lnbl9jcHVfaGVs cGVyKHZvaWQgKmluZm8pCiAgICAgICAgICAgICBjcHVtYXNrX2NsZWFyX2NwdShjcHUsICZjcHVw b29sX2ZyZWVfY3B1cyk7CiAgICAgICAgICAgICBnb3RvIG91dDsKICAgICAgICAgfQotICAgICAg ICBwZXJfY3B1KGNwdXBvb2wsIGNwdSkgPSBOVUxMOwogICAgICAgICBjcHVwb29sX21vdmluZ19j cHUgPSAtMTsKICAgICAgICAgY3B1cG9vbF9wdXQoY3B1cG9vbF9jcHVfbW92aW5nKTsKICAgICAg ICAgY3B1cG9vbF9jcHVfbW92aW5nID0gTlVMTDsKZGlmZiAtLWdpdCBhL3hlbi9jb21tb24vc2No ZWR1bGUuYyBiL3hlbi9jb21tb24vc2NoZWR1bGUuYwppbmRleCBkN2UyZDk4Li45MDcyNTQwIDEw MDY0NAotLS0gYS94ZW4vY29tbW9uL3NjaGVkdWxlLmMKKysrIGIveGVuL2NvbW1vbi9zY2hlZHVs ZS5jCkBAIC0xNDg2LDYgKzE0ODYsMTcgQEAgdm9pZCBfX2luaXQgc2NoZWR1bGVyX2luaXQodm9p ZCkKICAgICAgICAgQlVHKCk7CiB9CiAKKy8qCisgKiBNb3ZlIGEgcENQVSBvdXRzaWRlIG9mIHRo ZSBpbmZsdWVuY2Ugb2YgdGhlIHNjaGVkdWxlciBvZiBpdHMgY3VycmVudAorICogY3B1cG9vbCwg b3Igc3ViamVjdCBpdCB0byB0aGUgc2NoZWR1bGVyIG9mIGEgbmV3IGNwdXBvb2wuCisgKgorICog Rm9yIHRoZSBwQ1BVcyB0aGF0IGFyZSByZW1vdmVkIGZyb20gdGhlaXIgY3B1cG9vbCwgdGhlaXIg c2NoZWR1bGVyIGJlY29tZXMKKyAqICZvcHMgKHRoZSBkZWZhdWx0IHNjaGVkdWxlciwgc2VsZWN0 ZWQgYXQgYm9vdCwgd2hpY2ggYWxzbyBzZXJ2aWNlcyB0aGUKKyAqIGRlZmF1bHQgY3B1cG9vbCku IEhvd2V2ZXIsIGFzIHRoZXNlIHBDUFVzIGFyZSBub3QgcmVhbGx5IHBhcnQgb2YgYW55IHBvb2ws CisgKiB0aGVyZSB3b24ndCBiZSBhbnkgc2NoZWR1bGluZyBldmVudCBvbiB0aGVtLCBub3QgZXZl biBmcm9tIHRoZSBkZWZhdWx0CisgKiBzY2hlZHVsZXIuIEJhc2ljYWxseSwgdGhleSB3aWxsIGp1 c3Qgc2l0IGlkbGUgdW50aWwgdGhleSBhcmUgZXhwbGljaXRseQorICogYWRkZWQgYmFjayB0byBh IGNwdXBvb2wuCisgKi8KIGludCBzY2hlZHVsZV9jcHVfc3dpdGNoKHVuc2lnbmVkIGludCBjcHUs IHN0cnVjdCBjcHVwb29sICpjKQogewogICAgIHVuc2lnbmVkIGxvbmcgZmxhZ3M7CkBAIC0xNDk0 LDkgKzE1MDUsMjQgQEAgaW50IHNjaGVkdWxlX2NwdV9zd2l0Y2godW5zaWduZWQgaW50IGNwdSwg c3RydWN0IGNwdXBvb2wgKmMpCiAgICAgdm9pZCAqcHByaXYsICpwcHJpdl9vbGQsICp2cHJpdiwg KnZwcml2X29sZDsKICAgICBzdHJ1Y3Qgc2NoZWR1bGVyICpvbGRfb3BzID0gcGVyX2NwdShzY2hl ZHVsZXIsIGNwdSk7CiAgICAgc3RydWN0IHNjaGVkdWxlciAqbmV3X29wcyA9IChjID09IE5VTEwp ID8gJm9wcyA6IGMtPnNjaGVkOworICAgIHN0cnVjdCBjcHVwb29sICpvbGRfcG9vbCA9IHBlcl9j cHUoY3B1cG9vbCwgY3B1KTsKKworICAgIC8qCisgICAgICogcENQVXMgb25seSBtb3ZlIGZyb20g YSB2YWxpZCBjcHVwb29sIHRvIGZyZWUgKGkuZS4sIG91dCBvZiBhbnkgcG9vbCksCisgICAgICog b3IgZnJvbSBmcmVlIHRvIGEgdmFsaWQgY3B1cG9vbC4gSW4gdGhlIGZvcm1lciBjYXNlICh3aGlj aCBoYXBwZW5zIHdoZW4KKyAgICAgKiBjIGlzIE5VTEwpLCB3ZSB3YW50IHRoZSBDUFUgdG8gaGF2 ZSBiZWVuIG1hcmtlZCBhcyBmcmVlIGFscmVhZHksIGFzCisgICAgICogd2VsbCBhcyB0byBub3Qg YmUgdmFsaWQgZm9yIHRoZSBzb3VyY2UgcG9vbCBhbnkgbG9uZ2VyLCB3aGVuIHdlIGdldCB0bwor ICAgICAqIGhlcmUuIEluIHRoZSBsYXR0ZXIgY2FzZSAod2hpY2ggaGFwcGVucyB3aGVuIGMgaXMg YSB2YWxpZCBjcHVwb29sKSwgd2UKKyAgICAgKiB3YW50IHRoZSBDUFUgdG8gc3RpbGwgYmUgbWFy a2VkIGFzIGZyZWUsIGFzIHdlbGwgYXMgdG8gbm90IHlldCBiZSB2YWxpZAorICAgICAqIGZvciB0 aGUgZGVzdGluYXRpb24gcG9vbC4KKyAgICAgKi8KKyAgICBBU1NFUlQoYyAhPSBvbGRfcG9vbCAm JiAoYyAhPSBOVUxMIHx8IG9sZF9wb29sICE9IE5VTEwpKTsKKyAgICBBU1NFUlQoY3B1bWFza190 ZXN0X2NwdShjcHUsICZjcHVwb29sX2ZyZWVfY3B1cykpOworICAgIEFTU0VSVCgoYyA9PSBOVUxM ICYmICFjcHVtYXNrX3Rlc3RfY3B1KGNwdSwgb2xkX3Bvb2wtPmNwdV92YWxpZCkpIHx8CisgICAg ICAgICAgIChjICE9IE5VTEwgJiYgIWNwdW1hc2tfdGVzdF9jcHUoY3B1LCBjLT5jcHVfdmFsaWQp KSk7CiAKICAgICBpZiAoIG9sZF9vcHMgPT0gbmV3X29wcyApCi0gICAgICAgIHJldHVybiAwOwor ICAgICAgICBnb3RvIG91dDsKIAogICAgIGlkbGUgPSBpZGxlX3ZjcHVbY3B1XTsKICAgICBwcHJp diA9IFNDSEVEX09QKG5ld19vcHMsIGFsbG9jX3BkYXRhLCBjcHUpOwpAQCAtMTUyNCw2ICsxNTUw LDkgQEAgaW50IHNjaGVkdWxlX2NwdV9zd2l0Y2godW5zaWduZWQgaW50IGNwdSwgc3RydWN0IGNw dXBvb2wgKmMpCiAgICAgU0NIRURfT1Aob2xkX29wcywgZnJlZV92ZGF0YSwgdnByaXZfb2xkKTsK ICAgICBTQ0hFRF9PUChvbGRfb3BzLCBmcmVlX3BkYXRhLCBwcHJpdl9vbGQsIGNwdSk7CiAKKyBv dXQ6CisgICAgcGVyX2NwdShjcHVwb29sLCBjcHUpID0gYzsKKwogICAgIHJldHVybiAwOwogfQog Cg== --=-fiP2hMyb249kJUsRlMqH-- --=-IIqgnRvmB41h2mCwRkbt 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 v1 iEYEABECAAYFAlYnxwUACgkQk4XaBE3IOsQ3CACeIs2eS+13E8Ht2Mk6qEoh9nWz NI8AnjGg0x+2ubUiPKOK2BCgzKkdh7l4 =z1vQ -----END PGP SIGNATURE----- --=-IIqgnRvmB41h2mCwRkbt-- --===============4158183802285865648== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4158183802285865648==--