From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask() Date: Wed, 19 Mar 2014 19:54:41 +0100 Message-ID: <1395255281.13892.58.camel@Solace> References: <1395145511-13381-1-git-send-email-bob.liu@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8548216298952572728==" Return-path: In-Reply-To: <1395145511-13381-1-git-send-email-bob.liu@oracle.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: Bob Liu Cc: ian.campbell@citrix.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============8548216298952572728== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-iRwr2qo51ztvlULBTdV5" --=-iRwr2qo51ztvlULBTdV5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mar, 2014-03-18 at 20:25 +0800, Bob Liu wrote: > Introduce alloc_domheap_pages_nodemask() to allow specification of which = node(s) > to allocate memory from even when 'd =3D=3D NULL' is true. >=20 To me, this sentence would sound better, and more accurate wrt the implementation below, without the 'even'. Code wise... > Signed-off-by: Bob Liu > --- > xen/common/page_alloc.c | 25 +++++++++++++++++-------- > xen/include/xen/mm.h | 4 ++++ > 2 files changed, 21 insertions(+), 8 deletions(-) >=20 > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 601319c..85e8188 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -561,16 +561,18 @@ static void check_low_mem_virq(void) > static struct page_info *alloc_heap_pages( > unsigned int zone_lo, unsigned int zone_hi, > unsigned int order, unsigned int memflags, > - struct domain *d) > + struct domain *d, nodemask_t nodemask) > { > unsigned int first_node, i, j, zone =3D 0, nodemask_retry =3D 0; > unsigned int node =3D (uint8_t)((memflags >> _MEMF_node) - 1); > unsigned long request =3D 1UL << order; > struct page_info *pg; > - nodemask_t nodemask =3D (d !=3D NULL ) ? d->node_affinity : node_onl= ine_map; > bool_t need_tlbflush =3D 0; > uint32_t tlbflush_timestamp =3D 0; > =20 > + if (d !=3D NULL) > + nodemask =3D d->node_affinity; > + > if ( node =3D=3D NUMA_NO_NODE ) > { > memflags &=3D ~MEMF_exact_node; > ... If it were me, I probably would have added alloc_heap_pages_nodemask(...,nodemask_t nodemask) (similarly to what we do up here, plus the actual renaming to alloc_heap_pages_nodemask() ) and have the 'new' alloc_heap_pages() be a wrapper of that, with (d !=3D NULL ) ? d->node_affinity : node_online_map as the nodemask parameter. The reason is it looks cleaner and easier to use to me. In particular, what I dislike is this... > @@ -1338,7 +1340,7 @@ void *alloc_xenheap_pages(unsigned int order, unsig= ned int memflags) > ASSERT(!in_irq()); > =20 > pg =3D alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN, > - order, memflags, NULL); > + order, memflags, NULL, node_online_map); > ... the alloc_heap_pages(...,NULL,node_online_map) call, but that's mostly a matter of taste, I guess, so if maintainers are fine with the current approach, I certainly am too. The only thing I'm a bit concerned about is the fact that, either way, if one specifies a non NULL domain, the domain's node_affinity is used, and the nodemask ignored. I'm fine with it, but shouldn't this be a bit more evident (a comment, some if()/ASSERT, the changelog, etc.)? BTW, apart from these minor observation, and FWIW, this change looks fine to me. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-iRwr2qo51ztvlULBTdV5 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.0.22 (GNU/Linux) iEYEABECAAYFAlMp5/EACgkQk4XaBE3IOsQsYQCgpLiWfroBzNqkHplhnJLWKCyD xygAnA0GQ1ElgNeByyDwAHxqCTuKPmfd =Seil -----END PGP SIGNATURE----- --=-iRwr2qo51ztvlULBTdV5-- --===============8548216298952572728== 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 --===============8548216298952572728==--