Re: [libvirt] [PATCH 0/2] rework the check for the numa cpus

On 02.04.2015 10:02, Luyao Huang wrote: > We introduce a check for numa cpu total count in 5f7b71, > But seems this check cannot work well. There are some scenarioes: > > 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10): > > <vcpu placement='static'>10</vcpu> > <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/> > > the cpus '100' exceed maxvcpus, this setting is not valid (although qemu > do not output error). > > 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): > <vcpu placement='static'>10</vcpu> > <cell id='0' cpus='0-3' memory='512000' unit='KiB'/> > <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> > > I guess nobody will use this setting if he really want use numa in his > vm. (qemu not output error, but we can find some interesting things in > vm, all cpus have bind in one numa node) > > 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): > <vcpu placement='static'>10</vcpu> > <cell id='0' cpus='0-6' memory='512000' unit='KiB'/> > <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> > > No need forbid this scenario if scenario 2 is a 'valid' setting. > > However add new check during parse xml will make vm has broken settings > disappear after update libvirtd, so possible solutions: > > 1. add new check when parse xml, and find a way to avoid vm evaporate. > I chose this way and i don't think just drop the invalid settings is a good > idea for numa cpus, so i add a new function. > > 2. add new check when start vm. > I think this is a configuration issue, so no need report it so late. > > 3. just remove this check and do not add new check... :) > > Luyao Huang (2): > conf: introduce a new function to add check avoid losing track > conf: rework the cpu check for vm numa settings > > src/bhyve/bhyve_driver.c | 4 ++-- > src/conf/domain_conf.c | 21 ++++++++++++++------- > src/conf/domain_conf.h | 1 + > src/conf/numa_conf.c | 37 ++++++++++++++++++++++++++++++------- > src/conf/numa_conf.h | 2 +- > src/esx/esx_driver.c | 2 +- > src/libxl/libxl_driver.c | 4 ++-- > src/lxc/lxc_driver.c | 4 ++-- > src/openvz/openvz_driver.c | 4 ++-- > src/parallels/parallels_driver.c | 2 +- > src/phyp/phyp_driver.c | 2 +- > src/qemu/qemu_driver.c | 4 ++-- > src/test/test_driver.c | 4 ++-- > src/uml/uml_driver.c | 4 ++-- > src/vbox/vbox_common.c | 2 +- > src/vmware/vmware_driver.c | 4 ++-- > src/xen/xen_driver.c | 4 ++-- > src/xenapi/xenapi_driver.c | 4 ++-- > 18 files changed, 70 insertions(+), 39 deletions(-) > I agree with renaming and extending the virDomainNumaGetCPUCountTotal(). Even the diff in 2/2 shows the function is utterly broken. However, I think this function can be called unconditionally - even when the flag is not set. Having said that, the flag is redundant.

Moreover, when sending a patchset, the sources should compile cleanly after each patch. This is not the case with this one.

Looking forward to v2.

Michal