Re: [libvirt] [PATCHv4 3/3] qemu: Add args generation for file memory backing

-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka(a)intel.com>; libvir-list(a)redhat.com Cc: Mooney, Sean K <sean.k.mooney(a)intel.com>; Ptacek, MichalX <michalx.ptacek(a)intel.com&gt; Subject: Re: [libvirt] [PATCHv4 3/3] qemu: Add args generation for file memory backing On 13.12.2016 13:12, Jaroslav Safka wrote: > This patch add support for file memory backing on numa topology. > > The specified access mode in memoryBacking can be overriden by > specifying token memAccess in numa cell. > --- > src/qemu/qemu_command.c | 113 ++++++++++++++------- > .../qemuxml2argv-fd-memory-no-numa-topology.args | 21 ++++ > .../qemuxml2argv-fd-memory-no-numa-topology.xml | 27 +++++ > .../qemuxml2argv-fd-memory-numa-topology.args | 24 +++++ > .../qemuxml2argv-fd-memory-numa-topology.xml | 30 ++++++ > .../qemuxml2argv-fd-memory-numa-topology2.args | 26 +++++ > .../qemuxml2argv-fd-memory-numa-topology2.xml | 31 ++++++ > .../qemuxml2argv-fd-memory-numa-topology3.args | 30 ++++++ > .../qemuxml2argv-fd-memory-numa-topology3.xml | 32 ++++++ > tests/qemuxml2argvtest.c | 12 ++- > tests/qemuxml2xmltest.c | 1 - > 11 files changed, 308 insertions(+), 39 deletions(-) create mode > 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.xml A lot of tests. Impressive. > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index > 7d186d2..a897ed5 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3315,15 +3315,11 @@ qemuBuildMemoryBackendStr(unsigned long long size, > if (!(props = virJSONValueNewObject())) > return -1; > > - if (pagesize) { > - if (qemuGetDomainHupageMemPath(def, cfg, pagesize, &mem_path) < 0) > - goto cleanup; > - > + if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) { > *backendType = "memory-backend-file"; > > if (virJSONValueObjectAdd(props, > - "b:prealloc", true, > - "s:mem-path", mem_path, > + "s:mem-path", cfg->libDir, Really? cfg->libDir should stay intact. Should QEMU need to create a file, something cfg->stateDir based is probably more suitable. Or even some /tmp/ based path. Or am I misunderstanding something?

> NULL) < 0) > goto cleanup; > > @@ -3339,18 +3335,54 @@ qemuBuildMemoryBackendStr(unsigned long long size, > break; > > case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT: > + if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) { > + if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) > + goto cleanup; > + } > + break; > + > case VIR_DOMAIN_MEMORY_ACCESS_LAST: > break; > } > + > + force = true; > } else { > - if (memAccess) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Shared memory mapping is supported " > - "only with hugepages")); > - goto cleanup; Is this right? What if we are starting a domain that doesn't have hugepages nor def->mem.source = file set? I think this should be kept in. Moreover, because you removed this bit, you also had to remove the test from qemuxml2argv. Please don't remove tests that are failing after your change - they are failing for a very good reason.

> - } > + if (pagesize) { > + if (qemuGetDomainHupageMemPath(def, cfg, pagesize, &mem_path) < 0) > + goto cleanup; > > - *backendType = "memory-backend-ram"; > + *backendType = "memory-backend-file"; > + > + if (virJSONValueObjectAdd(props, > + "b:prealloc", true, > + "s:mem-path", mem_path, > + NULL) < 0) > + goto cleanup; > + > + switch (memAccess) { > + case VIR_DOMAIN_MEMORY_ACCESS_SHARED: > + if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) > + goto cleanup; > + break; > + > + case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE: > + if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0) > + goto cleanup; > + break; > + > + case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT: > + if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) { > + if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) > + goto cleanup; > + } > + break; > + > + case VIR_DOMAIN_MEMORY_ACCESS_LAST: > + break; > + } > + } else { > + *backendType = "memory-backend-ram"; > + } > } Oh, this is just a 1:1 copy of what is already there and what you know use for def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE case. I think these to branches can be merged together and all that you need to special case (i.e. mem_path and 'force = true;' can be special cased in the body: if (pagesize || def->mem.source ...) { if (pagesize) { /* use qemuGetDomainHupageMemPath to look up mem-path */ } else { VIR_STRDUP(mem_path, /* some per-domain? path */ } *backendType = file; ... if (def->mem.source..) force = true; } else { *backendType = ram; }

> > if (virJSONValueObjectAdd(props, "U:size", size * 1024, NULL) < > 0) @@ -7250,31 +7282,37 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, > const long system_page_size = virGetSystemPageSizeKB(); > char *mem_path = NULL; > > - /* > - * No-op if hugepages were not requested. > - */ > - if (!def->mem.nhugepages) > - return 0; > + if (def->mem.nhugepages) { > > - /* There is one special case: if user specified "huge" > - * pages of regular system pages size. > - * And there is nothing to do in this case. > - */ > - if (def->mem.hugepages[0].size == system_page_size) > - return 0; > + /* There is one special case: if user specified "huge" > + * pages of regular system pages size. > + * And there is nothing to do in this case. > + */ > + if (def->mem.hugepages[0].size == system_page_size) > + return 0; > > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("hugepage backing not supported by '%s'"), > - def->emulator); > - return -1; > - } > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("hugepage backing not supported by '%s'"), > + def->emulator); > + return -1; > + } > > - if (qemuGetDomainHupageMemPath(def, cfg, def- >mem.hugepages[0].size, &mem_path) < 0) > - return -1; > + if (qemuGetDomainHupageMemPath(def, cfg, def- >mem.hugepages[0].size, &mem_path) < 0) > + return -1; > + > + if (def->mem.allocation != VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) > + virCommandAddArgList(cmd, "-mem-prealloc", NULL); > + > + virCommandAddArgList(cmd, "-mem-path", mem_path, NULL); > + VIR_FREE(mem_path); > + } else { > + /* > + * No-op if hugepages or immediate allocation were not requested. > + */ > + return 0; > + } > > - virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL); > - VIR_FREE(mem_path); > > return 0; > } Again, a lot of useless changes because you have changed one condition. I mean, this function is still no-op if no hugepages are requested, but instead of returning early, the actual return for the negative case is here. This would be so tiny, easy peasy change had you not inverted the condition.

> @@ -7303,9 +7341,12 @@ qemuBuildMemCommandLine(virCommandPtr cmd, > virDomainDefGetMemoryInitial(def) / 1024); > } > > + if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) > + virCommandAddArgList(cmd, "-mem-prealloc", NULL); > + > /* > - * Add '-mem-path' (and '-mem-prealloc') parameter here only if > - * there is no numa node specified. > + * Add '-mem-path' (and '-mem-prealloc') parameter here if > + * the hugepages and no numa node is specified. > */ > if (!virDomainNumaGetNodeCount(def->numa) && > qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index > 81c62ac..babff8f 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1500,8 +1500,6 @@ mymain(void) > DO_TEST_PARSE_ERROR("cpu-numa3", NONE); > DO_TEST_FAILURE("cpu-numa-disjoint", NONE); > DO_TEST("cpu-numa-disjoint", QEMU_CAPS_NUMA); > - DO_TEST_FAILURE("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_RAM); > - DO_TEST_FAILURE("cpu-numa-memshared", NONE); NACK to this chunk. > DO_TEST("cpu-host-model", NONE); > DO_TEST("cpu-host-model-vendor", NONE); > skipLegacyCPUs = true; > @@ -2415,6 +2413,16 @@ mymain(void) > > DO_TEST("cpu-hotplug-startup", > QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); > > + DO_TEST("fd-memory-numa-topology", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE, > + QEMU_CAPS_KVM); > + DO_TEST("fd-memory-numa-topology2", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE, > + QEMU_CAPS_KVM); > + DO_TEST("fd-memory-numa-topology3", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE, > + QEMU_CAPS_KVM); > + > + DO_TEST("fd-memory-no-numa-topology", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE, > + QEMU_CAPS_KVM); > + > qemuTestDriverFree(&driver); > > return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; diff --git > a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index > e4d011f..ae6a873 100644 > --- a/tests/qemuxml2xmltest.c > +++ b/tests/qemuxml2xmltest.c > @@ -939,7 +939,6 @@ mymain(void) > DO_TEST("cpu-numa-no-memory-element", NONE); > DO_TEST("cpu-numa-disordered", NONE); > DO_TEST("cpu-numa-disjoint", NONE); > - DO_TEST("cpu-numa-memshared", NONE); NACK to this chunk. Removing tests because they are failing after a change is/should be no-no. > > DO_TEST("numatune-auto-prefer", NONE); > DO_TEST("numatune-memnode", NONE); > Otherwise looking good. Michal