From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 10/16] xen: sched: use soft-affinity instead of domain's node-affinity Date: Fri, 15 Nov 2013 01:39:49 +0100 Message-ID: <1384475989.16918.93.camel@Solace> References: <20131113190852.18086.5437.stgit@Solace> <20131113191233.18086.60472.stgit@Solace> <5284EC99.3070607@eu.citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0171801096744481774==" Return-path: In-Reply-To: <5284EC99.3070607@eu.citrix.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: George Dunlap Cc: Marcus Granado , Keir Fraser , Ian Campbell , Li Yechen , Andrew Cooper , Juergen Gross , Ian Jackson , xen-devel@lists.xen.org, Jan Beulich , Justin Weaver , Matt Wilson , Elena Ufimtseva List-Id: xen-devel@lists.xenproject.org --===============0171801096744481774== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-8WOh9FBuXGqeTapKbUUi" --=-8WOh9FBuXGqeTapKbUUi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On gio, 2013-11-14 at 15:30 +0000, George Dunlap wrote: > On 13/11/13 19:12, Dario Faggioli wrote: > > [..] > > The high level description of NUMA placement and scheduling in > > docs/misc/xl-numa-placement.markdown is being updated too, to match > > the new architecture. > > > > signed-off-by: Dario Faggioli >=20 > Reviewed-by: George Dunlap >=20 Cool, thanks. > Just a few things to note below... >=20 Ok. > > diff --git a/xen/common/domain.c b/xen/common/domain.c > > @@ -411,8 +411,6 @@ void domain_update_node_affinity(struct domain *d) > > node_set(node, d->node_affinity); > > } > > =20 > > - sched_set_node_affinity(d, &d->node_affinity); > > - > > spin_unlock(&d->node_affinity_lock); >=20 > At this point, the only thing inside the spinlock is contingent on=20 > d->auto_node_affinity. >=20 Mmm... Sorry, but I'm not geting what you mean here. :-( > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > > -static inline int __vcpu_has_node_affinity(const struct vcpu *vc, > > +static inline int __vcpu_has_soft_affinity(const struct vcpu *vc, > > const cpumask_t *mask) > > { > > - const struct domain *d =3D vc->domain; > > - const struct csched_dom *sdom =3D CSCHED_DOM(d); > > - > > - if ( d->auto_node_affinity > > - || cpumask_full(sdom->node_affinity_cpumask) > > - || !cpumask_intersects(sdom->node_affinity_cpumask, mask) ) > > + if ( cpumask_full(vc->cpu_soft_affinity) > > + || !cpumask_intersects(vc->cpu_soft_affinity, mask) ) > > return 0; >=20 > At this point we've lost a way to make this check potentially much=20 > faster (being able to check auto_node_affinity). > Right. > This isn't a super-hot=20 > path but it does happen fairly frequently --=20 > Quite frequently indeed. > will the "cpumask_full()"=20 > check take a significant amount of time on, say, a 4096-core system? If= =20 > so, we might think about "caching" the results of cpumask_full() at some= =20 > point. >=20 Yes, I think cpumask_* operation could be heavy when the number of pcpus is high. However, this is not really a problem introduced by this series. Consider that the default behavior (for libxl and xl) is to go through initial domain placement, which would set a node-affinity for the domain explicitly, which means d->auto_node_affinity is false. In fact, every domain that does not manually pin its vcpus at creation time --which is what we want, because that way NUMA placement can do its magic-- will have to go through the (cpumask_full || !cpumask_intrscts) anyway. Basically, I'm saying that having d->auto_node_affinity there may look like a speedup, but it really is only for a minority of cases. So, yes, I think we should aim at optimizing this, but that is something completely orthogonal to this series. That is to say: (a) we should do it anyway, whether or not this series goes in; (b) for that same reason, that shouldn't prevent this series from going in. If you think this can be an issue for 4.4, I'm fine creating a bug for it and putting it among the blockers. At that point, I'll start looking for a solution, and will commit to post a fix ASAP, but again, that's pretty independent from this very series, at least AFAICT. Then, the fact that you provided your Reviewed-by above probably means that you are aware and ok with this all, but I felt like it was worth pointing it out anyway. :-) 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) --=-8WOh9FBuXGqeTapKbUUi 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.4.15 (GNU/Linux) iEYEABECAAYFAlKFbVUACgkQk4XaBE3IOsSc9wCeMRqr3nZTq/1RCWFS3EoBpnk2 gBIAnAy+QhM/6y+hzzsRbdR9l2md7R24 =SROl -----END PGP SIGNATURE----- --=-8WOh9FBuXGqeTapKbUUi-- --===============0171801096744481774== 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 --===============0171801096744481774==--