Re: [Xen-devel] [RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask()

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel