From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 4/4] xen: sched: get rid of cpupool_scheduler_cpumask() Date: Fri, 3 Jul 2015 09:48:38 +0200 Message-ID: <1435909718.14347.53.camel@citrix.com> References: <20150625103457.3353.39292.stgit@Solace.station> <20150625121534.3353.10992.stgit@Solace.station> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7736041870999536646==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZAviR-0005Un-BC for xen-devel@lists.xenproject.org; Fri, 03 Jul 2015 07:48:55 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Juergen Gross , Sisu Xi , Robert VanVossen , Josh Whitehead , Meng Xu , xen-devel List-Id: xen-devel@lists.xenproject.org --===============7736041870999536646== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-ZQkgMh0/5QDdiKPjNtWg" --=-ZQkgMh0/5QDdiKPjNtWg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2015-07-02 at 16:39 +0100, George Dunlap wrote: > On Thu, Jun 25, 2015 at 1:15 PM, Dario Faggioli > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > > index a1945ac..8c36635 100644 > > --- a/xen/common/sched_credit.c > > +++ b/xen/common/sched_credit.c > > @@ -374,7 +374,7 @@ __runq_tickle(unsigned int cpu, struct csched_vcpu = *new) > > > > /* cpu is vc->processor, so it must be in a cpupool. */ > > ASSERT(per_cpu(cpupool, cpu) !=3D NULL); > > - online =3D cpupool_online_cpumask(per_cpu(cpupool, cpu)); > > + online =3D cpupool_domain_cpumask(new->sdom->dom); >=20 > Two comments in passing here (no action required): >=20 > 1. Looks like passing both cpu and svc to this function is a bit > redundant, as the function can just look up new->vcpu->processor > itself >=20 Indeed! It's not only redundant, it's disturbing, IMO. In fact, ever time I look at it, I wander what cpu is, because, if it just was ->processor, it wouldn't be necessary for it to be a parameter... then I smack my head and remember that, yes, it's _that_ function! So, yes, I'm all for killing it! > 2. After this patch, the ASSERT there will be redundant, as there's > already an identical assert in cpupool_domain_cpumask() >=20 > Those won't hurt, but if you end up respinning you might think about > at least taking the ASSERT out. >=20 Right, I'll take it out. > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > > index 4ffcd98..7ad298a 100644 > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -80,7 +80,7 @@ static struct scheduler __read_mostly ops; > > > > #define DOM2OP(_d) (((_d)->cpupool =3D=3D NULL) ? &ops : ((_d)->cpu= pool->sched)) > > #define VCPU2OP(_v) (DOM2OP((_v)->domain)) > > -#define VCPU2ONLINE(_v) cpupool_online_cpumask((_v)->domain->cpupool) > > +#define VCPU2ONLINE(_v) cpupool_domain_cpumask((_v)->domain) > > > > static inline void trace_runstate_change(struct vcpu *v, int new_state= ) > > { > > diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h > > index 7cc25c6..20af791 100644 > > --- a/xen/include/xen/sched-if.h > > +++ b/xen/include/xen/sched-if.h > > @@ -183,9 +183,17 @@ struct cpupool > > atomic_t refcnt; > > }; > > > > -#define cpupool_scheduler_cpumask(_pool) \ > > - (((_pool) =3D=3D NULL) ? &cpupool_free_cpus : (_pool)->cpu_valid) > > #define cpupool_online_cpumask(_pool) \ > > (((_pool) =3D=3D NULL) ? &cpu_online_map : (_pool)->cpu_valid) > > > > +static inline cpumask_t* cpupool_domain_cpumask(struct domain *d) > > +{ > > + /* > > + * d->cpupool =3D=3D NULL only for the idle domain, which should > > + * have no interest in calling into this. > > + */ > > + ASSERT(d->cpupool !=3D NULL); > > + return cpupool_online_cpumask(d->cpupool); > > +} >=20 > It's a bit strange to assert that d->cpupool !=3D NULL, and then call a > macro whose return value only depends on whether d->cpupool is NULL or > not. Why not just return d->cpupool->cpu_valid? >=20 Yes, you're right. I think this is because my original intent was to replace cpupool_scheduler_cpumask() with *something* built on top of cpupool_online_cpumask(), and I retained this design, even when it became something that could just be 'return d->cpupool->cpu_valid'! :-) I'll change this. > Thanks for tracking these down, BTW! >=20 It was a bit of a nightmare, actually, to (1) figure out what was going on, and (2) come up with a satisfying fix... But, yes, it was well worth. :-) 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) --=-ZQkgMh0/5QDdiKPjNtWg 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 iEYEABECAAYFAlWWPl8ACgkQk4XaBE3IOsSzTQCfXdIuK9F3Fp9Yq8t6FroVTTQb 0AoAn31IY5JKUhNJyYExZA+LPjLSd3H+ =+Z2T -----END PGP SIGNATURE----- --=-ZQkgMh0/5QDdiKPjNtWg-- --===============7736041870999536646== 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 --===============7736041870999536646==--