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