[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]