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: Thu, 15 Oct 2015 10:53:41 +0200 Message-ID: <1444899221.3009.76.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="===============3627620505898990739==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZmeIG-0000pY-4H for xen-devel@lists.xenproject.org; Thu, 15 Oct 2015 08:53:48 +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 --===============3627620505898990739== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-Lq4o7PVtFmbwVUy+mpuj" --=-Lq4o7PVtFmbwVUy+mpuj Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2015-10-15 at 10:25 +0200, Juergen Gross wrote: > On 10/14/2015 05:54 PM, Dario Faggioli wrote: > > schedule_cpu_switch() is meant to be only used for moving > > pCPUs from a cpupool to no cpupool, and from there back > > to a cpupool, *not* to move them directly from one cpupool > > to another. > >=20 > > This is something that is reflected in the way it is > > implemented, and should be kept in mind when looking at > > it. However, that is not that clear, by just the look of > > it. > >=20 > > Make it more evident by adding commentary and ASSERT()s. > >=20 > > Signed-off-by: Dario Faggioli > > --- > > Cc: George Dunlap > > Cc: Juergen Gross > > Cc: Jan Beulich > > --- > > Changes from v1: > > * new patch, was not there in v1. > > --- > > xen/common/schedule.c | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > >=20 > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > > index 9aa209d..5ebfa33 100644 > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -1486,12 +1486,40 @@ 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 becomes > > + * &ops (the default scheduler, selected at boot, which also > > services the > > + * default cpupool). However, as these pCPUs are not really part > > of any pool, > > + * 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) > > { > > struct vcpu *idle; > > 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 *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 valid > > + * for the destination pool. > > + * This all is because we do not want any scheduling activity > > on the CPU > > + * while, in here, we switch things over. >=20 > While this is correct I think you should mention that in the "adding > to > a pool" case per_cpu(cpupool, cpu) already contains the new pool > pointer. So c =3D=3D pool then and both are not NULL. >=20 Eheh, I did have an ASSERT() for this too, but I eventually dropped it, as it looked a bit too verbose. :-) I can add it, but I like this suggestion of yours here below, so I guess I'll try it first... > 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 Yep, as said, in principle, I like the idea. I'll try and see how the code end up looking like. > 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'm not entirely sure what you mean when you refer to "taking the scheduler lock", but yes, I'll keep the ordering. 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) --=-Lq4o7PVtFmbwVUy+mpuj 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 iEYEABECAAYFAlYfaZUACgkQk4XaBE3IOsQvFwCdGbQE+jLHZpYOMsoYzKq1BCFX AJ0AnjdW3L2zCn9NlfnTwnjRu0gyr5/R =G0nb -----END PGP SIGNATURE----- --=-Lq4o7PVtFmbwVUy+mpuj-- --===============3627620505898990739== 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 --===============3627620505898990739==--