Re: [PATCH v5 06/24] libxl: introduce vNUMA types

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Feb 16, 2015 at 02:58:32PM +0000, Dario Faggioli wrote:
> On Thu, 2015-02-12 at 19:44 +0000, Wei Liu wrote:
> > A domain can contain several virtual NUMA nodes, hence we introduce an
> > array in libxl_domain_build_info.
> > 
> > libxl_vnode_info contains the size of memory in that node, the distance
> > from that node to every nodes, the underlying pnode and a bitmap of
> > vcpus.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Dario Faggioli <dario.faggioli@citrix.com>
> > Cc: Elena Ufimtseva <ufimtseva@gmail.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> This looks fine, and matches what we agreed upon for this interface, a
> few months back, so:
> 
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Just one comment.
> 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 02be466..14c7e7c 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -356,6 +356,13 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
> >      ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> >      ])
> >  
> > +libxl_vnode_info = Struct("vnode_info", [
> > +    ("memkb", MemKB),
> > +    ("distances", Array(uint32, "num_distances")), # distances from this node to other nodes
> > +    ("pnode", uint32), # physical node of this node
> >
> I am unsure whether we ever discussed this already or not (and sorry for
> not recalling) but, in principle, one vnode can be mapped to more than
> just one pnode.
> 

I don't recall either.

> Semantic would be that the memory of the vnode is somehow split (evenly,
> by default, I would say) between the specified pnodes. So, pnode could
> be a bitmap too (and be called "pnodes" :-) ), although we can put
> checks in place that --for now-- it always have only one bit set.
> 
> Reasons might be that the user just wants it, or that there is not
> enough (free) memory on just one pnode, but we still want to achieve
> some locality.
> 

Wouldn't this cause unpredictable performance? And there is no way to
specify priority among the group of nodes you specify with a single
bitmap.

I can't say I fully understand the implication of the scenario you
described.

Wei.

> This is probably something we can change/add later, but since it affects
> the interface, I felt like saying it as soon as it came to my mind.
> 
> Regards,
> Dario