Re: [libvirt] [PATCHv3.5 0/4] Automaticaly fill <memory> element for NUMA enabled guests

On 03/06/2015 09:40 AM, Peter Krempa wrote: > Pavel's series changed few same places thus the previous version no longer applies. > > Peter Krempa (4): > conf: Replace access to def->mem.max_balloon with accessor functions > qemu: command: Add helper to align memory sizes > conf: Automatically use NUMA memory size in case NUMA is enabled > conf: Make specifying <memory> optional > > docs/schemas/domaincommon.rng | 18 ++--- > src/conf/domain_conf.c | 81 +++++++++++++++++++--- > src/conf/domain_conf.h | 4 ++ > src/hyperv/hyperv_driver.c | 2 +- > src/libvirt_private.syms | 3 + > src/libxl/libxl_conf.c | 2 +- > src/libxl/libxl_driver.c | 8 +-- > src/lxc/lxc_cgroup.c | 2 +- > src/lxc/lxc_driver.c | 12 ++-- > src/lxc/lxc_fuse.c | 4 +- > src/lxc/lxc_native.c | 4 +- > src/openvz/openvz_driver.c | 2 +- > src/parallels/parallels_driver.c | 2 +- > src/parallels/parallels_sdk.c | 12 ++-- > src/phyp/phyp_driver.c | 11 +-- > src/qemu/qemu_command.c | 23 +++--- > src/qemu/qemu_domain.c | 21 ++++++ > src/qemu/qemu_domain.h | 2 + > src/qemu/qemu_driver.c | 21 +++--- > src/qemu/qemu_hotplug.c | 8 ++- > src/qemu/qemu_process.c | 2 +- > src/test/test_driver.c | 8 +-- > src/uml/uml_driver.c | 8 +-- > src/vbox/vbox_common.c | 4 +- > src/vmware/vmware_driver.c | 2 +- > src/vmx/vmx.c | 12 ++-- > src/xen/xm_internal.c | 14 ++-- > src/xenapi/xenapi_driver.c | 2 +- > src/xenapi/xenapi_utils.c | 4 +- > src/xenconfig/xen_common.c | 8 ++- > src/xenconfig/xen_sxpr.c | 9 +-- > .../qemuxml2argv-cpu-numa-no-memory-element.args | 7 ++ > .../qemuxml2argv-cpu-numa-no-memory-element.xml | 24 +++++++ > .../qemuxml2argv-minimal-no-memory.xml | 25 +++++++ > .../qemuxml2argv-numatune-memnode.args | 2 +- > tests/qemuxml2argvtest.c | 2 + > .../qemuxml2xmlout-cpu-numa-no-memory-element.xml | 28 ++++++++ > tests/qemuxml2xmltest.c | 1 + > 38 files changed, 299 insertions(+), 105 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-no-memory-element.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-no-memory-element.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-minimal-no-memory.xml > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-numa-no-memory-element.xml > Since this is the order of your repo on git://pipo.sk/pipo/libvirt.git, I'll start with these. Couple of general comments... - I think the new function names are better - I think one can tell that the "KiB" or "MiB" was realized at some point in time after "KB" and "MB" - as there's stray comments and variables using the (I guess) now incorrect terminology. Not that I'm asking for them to be changed, just an "observation". - Whether they snuck in after you started this - there are a few direct references to mem.max_balloon in the bhyve_command.c. Should they be replaced by the new accessors?

- Because it became apparent in patch 4... In virDomainDefParseXML, just to be "complete" shouldn't the: /* Extract domain memory */ if (virDomainParseMemory("./memory[1]", NULL, ctxt, &def->mem.max_balloon, false, true) < 0) be replaced with something like: unsigned long long memory_value; ... /* Extract domain memory */ if (virDomainParseMemory("./memory[1]", NULL, ctxt, &memory_value, false, true) < 0) { ... } virDomainDefSetMemoryInitial(def, memory_value); Although it seems like overkill - it's just making sure no one tries to cut-copy-paste in the future.

- With having virDomainDefGetMemoryInitial understand NUMA vs Balloon, should virDomainDefSetMemoryInitial check and not set max_balloon? Theoretically if <memory> wasn't filled, then we're possible setting something we shouldn't which could then be output when the domain is saved, right?

ACK in principal - perhaps give it a day to see if anyone else has comments or issues...

John