[libvirt] Re：Re: [PATCH] add nodeset='all' and default for interleavemode

On 09/11/2018 04:28 PM, Peng Hao wrote: > For interleave mode,sometimes we want to allocate mmeory regularly > in all nodes on the host. But different hosts has different node number. > So we add nodeset='all' for interleave mode and if nodeset=NULL default > nodeset is 'all' for interleave mode. > > Signed-off-by: Peng Hao <peng.hao2(a)zte.com.cn&gt; > --- > src/conf/numa_conf.c | 73 ++++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 57 insertions(+), 16 deletions(-) Firstly, this patch does not pass 'syntax-check'. Secondly, it breaks qemuxml2argvtest.

> + numa->memory.allnode = true; > + } else { Any patch that changes accepted XML needs to go hand in hand with documentation and RNG update and a test case.

> + if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC && > + mode == VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE && > + numa->memory.allnode) { > + if ((bitmap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) == NULL) > + goto cleanup; > + virBitmapClearAll(bitmap); > + maxnode = numa_max_node();

So, you're including numa.h to get this function. What if:

a) numa.h is not available? b) what is wrong with virNumaGetMaxNode()?

> + for (i = 0; i <= maxnode; i++) { > + if (virBitmapSetBit(bitmap, i) < 0) { > + virBitmapFree(bitmap); > + goto cleanup; > + } > + } > + if (numa->memory.nodeset) > + virBitmapFree(numa->memory.nodeset); > + numa->memory.nodeset = bitmap; > } > > /* setting nodeset when placement auto is invalid */ >

But more importantly, why is this patch needed? I might be missing something, but:

a) you can just not pin the memory to avoid mismatch of NUMA nodes on migration, b) supply new domain XML on migration where NUMA nodes match the destination

Isn't pinning memory to all NUMA nodes equivalent to no pinning at all?

Michal