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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list