Comments inside > -----Original Message----- > From: Michal Privoznik [mailto:mprivozn@xxxxxxxxxx] > Sent: Saturday, January 28, 2017 3:03 PM > To: Safka, JaroslavX <jaroslavx.safka@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Mooney, Sean K <sean.k.mooney@xxxxxxxxx>; Ptacek, MichalX > <michalx.ptacek@xxxxxxxxx> > Subject: Re: [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? > [Jarek] You are right, I was not fully sure about this path. I will use the stateDir then. > > 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. > [Jarek] I will check it > > - } > > + 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; > } > [Jarek] I will try to change it. Thanks > > > > 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. > [Jarek] oki, I will change it > > @@ -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 -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
