[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask()
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 == NULL' is true. > To me, this sentence would sound better, and more accurate wrt the implementation below, without the 'even'. Code wise... > Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx> > --- > xen/common/page_alloc.c | 25 +++++++++++++++++-------- > xen/include/xen/mm.h | 4 ++++ > 2 files changed, 21 insertions(+), 8 deletions(-) > > 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 = 0, nodemask_retry = 0; > unsigned int node = (uint8_t)((memflags >> _MEMF_node) - 1); > unsigned long request = 1UL << order; > struct page_info *pg; > - nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map; > bool_t need_tlbflush = 0; > uint32_t tlbflush_timestamp = 0; > > + if (d != NULL) > + nodemask = d->node_affinity; > + > if ( node == NUMA_NO_NODE ) > { > memflags &= ~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 != 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, unsigned > int memflags) > ASSERT(!in_irq()); > > pg = 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 -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |