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

[lhuang]

On 04/20/2015 10:39 PM, Michal Privoznik wrote:
> 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.

Oh, good news to me :)

So if this function can be called unconditionally, there is no reason to 
introduce this new flag.

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

Get it, i will pay more attention for this things next time, thanks for 
pointing out that.

> Looking forward to v2.

Thanks for your review and advise, i will propose a new version these days.

> Michal

Luyao