Re: [PATCH v10 5/9] libxl: vnuma types declararion

[Elena Ufimtseva <ufimtseva@gmail.com>]

On Wed, Sep 3, 2014 at 12:04 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-09-03 at 00:24 -0400, Elena Ufimtseva wrote:
>> Adds vnuma topology types declarations to libxl_domain_build_info
>> structure.
>
> So, as mentioned I was becoming increasingly confused about what all
> these fields represent. So lets stop and take stock.

That is not good )
I guess I did not understand correctly about new convention you have
mentioned in previous review.
But I know I need to have it re-worked, so let me try to answer.

>
>> Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
>> ---
>>  tools/libxl/libxl_types.idl |    8 +++++++-
>>  tools/libxl/libxl_vnuma.h   |   16 ++++++++++++++++
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>  create mode 100644 tools/libxl/libxl_vnuma.h
>>
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 08a7927..ea8bac0 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -333,7 +333,13 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("disable_migrate", libxl_defbool),
>>      ("cpuid",           libxl_cpuid_policy_list),
>>      ("blkdev_start",    string),
>> -
>> +    ("vnodes",          uint32),
>
> I suppose this is the number of NUMA nodes the guest should have?

Yes.

>
>> +    ("vmemranges",      uint32),
>
> and I suppose this is a number of memory ranges? Does this differ ever
> from vnodes?

Yes it can be different, it can as one node can have more than one memory range.
Parsing does not take this into account and further in the tool stack
there is no pretty way to work around this (see at the end).

>
>> +    ("vnuma_mem",       Array(uint64, "num_vnuma_mem")),
>
> The size of each memory node?

Yes.
>
> When/how can num_vnuma_mem differ from vmemranges?

Well, I did not work on it yet as vmemranges are not supported yet,
but will be needed for multi-range memory nodes.
But I agree, it does not look right to me as well.

>
>> +    ("vnuma_vcpumap",   Array(uint32, "num_vnuma_vcpumap")),
>
> Is this supposed to map numa node to a bitmap of vcpus? If yes:

It is a map, but not a bitmap of vcpus. Its an array of nr_vcpus size,
each element is the node number.
>
> When/how can num_vnuma_vcpumap differ from vnodes?

num_vnuma_vcpumap is the number of vcpus in map from config or by
default number of vcpus per domain.

>
> What happens if a guest has 33 vcpus?
>
> Or maybe the output of this is a single vcpu number?
>
>> +    ("vdistance",       Array(uint32, "num_vdistance")),
>
> nodes to distances? Can num_vdistance ever differ from vnodes?

num_vdistance is a square of vnodes.
>
> I thought distances were between two nodes, in which case doesn't this
> need to be multidimensional?

yes, but parser just fills it on per-row order, treating it as it is a
multidimensional array.

>
>> +    ("vnuma_vnodemap",  Array(uint32, "num_vnuma_vnondemap")),
>
> I've run out of guesses for what this might be. vnode to pnode perhaps?

Yes, it it vnode to pnode map. I guess me changing the field names
brought lots of confusion.


> You appear to unconditionally overwrite whatever the users might have
> put here.
>
>> +    ("vnuma_autoplacement",  libxl_defbool),
>
> Why/how does this differ from the existing numa placement option?


Basically, its meaning can be briefly described as to try to build
vnode to pnode mapping if automatic vnuma placement
enabled, or try to use the user-defined one.
But I am thinking maybe you are right, and there is no need in that
vnuma_autoplacement at all.
Just to remove it, and if numa placement is in automatic mode, just
use it. If not, try to use vnode to pnode mapping.

Now I think that is what Dario was talking about for some time )


Ok, now going back to these types.

  ("vnodes",          uint32),
  ("vmemranges",      uint32),
  ("vnuma_mem",       Array(uint64, "num_vnuma_mem")),
  ("vnuma_vcpumap",   Array(uint32, "num_vnuma_vcpumap")),
  ("vdistance",       Array(uint32, "num_vdistance")),
  ("vnuma_vnodemap",  Array(uint32, "num_vnuma_vnondemap")),


vnodes -  number of vnuma nodes;
vmemranges - number of vmemranges (not used, but added support in
Xen); Not sure how to deal with this. Should I just remove it as its
not used by tool stack yet?

vnuma_mem - vnode memory sizes (conflicts now with vmemranges);
vnuma_vcpumap - map of vnuma nodes and vcpus (length is number of
vcpus, each element is vnuma node number);
vdistance - distances, num_vdistance = vnodes * vnodes;
vnuma_vnodemap - mapping of vnuma nodes to physical NUMA nodes;

And vnuma_autoplacement probably not needed here at all.

About vmemranges. They will be needed for vnuma nodes what have
multiple ranges of memories.
I added it in type definition to specify support when calling
hypercall. As of now, PV domain have one memory range per node.

There is libxl__build_vnuma_ranges function which for PV domain
basically converts vnuma memory node sizes into ranges.
But for PV domain there is only one range per vnode.

Maybe there is a better way of doing this, maybe even remove confusing
at this point vmemranges?


Elena
>
> Ian.
>



-- 
Elena