[libvirt] [PATCHv4 0/3] Add support for file memorybacking
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology. <memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking> If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,share=on The access mode can be overriden by specifying token memAccess in numa cell. The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore Jaroslav Safka (3): qemu,conf: Rename virNumaMemAccess to virDomainMemoryAccess conf: Add new xml elements for file memorybacking support qemu: Add args generation for file memory backing docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 30 +++++ src/conf/domain_conf.c | 133 ++++++++++++++++----- src/conf/domain_conf.h | 22 ++++ src/conf/numa_conf.c | 15 +-- src/conf/numa_conf.h | 14 +-- src/qemu/qemu_command.c | 123 ++++++++++++------- src/qemu/qemu_process.c | 2 +- .../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 +++++ .../qemuxml2argv-memorybacking-set.xml | 24 ++++ .../qemuxml2argv-memorybacking-unset.xml | 24 ++++ tests/qemuxml2argvtest.c | 12 +- .../qemuxml2xmlout-memorybacking-set.xml | 32 +++++ .../qemuxml2xmlout-memorybacking-unset.xml | 32 +++++ tests/qemuxml2xmltest.c | 4 +- 22 files changed, 605 insertions(+), 92 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 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml -- 2.7.4 -------------------------------------------------------------- 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.
Rename to avoid duplicate code. Because virDomainMemoryAccess will be used in memorybacking for setting default behaviour. NOTE: The enum cannot be moved to qemu/domain_conf because of headers dependency --- src/conf/numa_conf.c | 15 ++++++++------- src/conf/numa_conf.h | 14 +++++++------- src/qemu/qemu_command.c | 10 +++++----- src/qemu/qemu_process.c | 2 +- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 9818d95..bfd3703 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -43,10 +43,11 @@ VIR_ENUM_IMPL(virDomainNumatunePlacement, "static", "auto"); -VIR_ENUM_IMPL(virNumaMemAccess, VIR_NUMA_MEM_ACCESS_LAST, +VIR_ENUM_IMPL(virDomainMemoryAccess, VIR_DOMAIN_MEMORY_ACCESS_LAST, "default", "shared", - "private"); + "private") + typedef struct _virDomainNumaNode virDomainNumaNode; typedef virDomainNumaNode *virDomainNumaNodePtr; @@ -64,7 +65,7 @@ struct _virDomainNuma { virBitmapPtr cpumask; /* bitmap of vCPUs corresponding to the node */ virBitmapPtr nodeset; /* host memory nodes where this guest node resides */ virDomainNumatuneMemMode mode; /* memory mode selection */ - virNumaMemAccess memAccess; /* shared memory access configuration */ + virDomainMemoryAccess memAccess; /* shared memory access configuration */ } *mem_nodes; /* guest node configuration */ size_t nmem_nodes; @@ -777,7 +778,7 @@ virDomainNumaDefCPUParseXML(virDomainNumaPtr def, goto cleanup; if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { - if ((rc = virNumaMemAccessTypeFromString(tmp)) <= 0) { + if ((rc = virDomainMemoryAccessTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid 'memAccess' attribute value '%s'"), tmp); @@ -803,7 +804,7 @@ int virDomainNumaDefCPUFormat(virBufferPtr buf, virDomainNumaPtr def) { - virNumaMemAccess memAccess; + virDomainMemoryAccess memAccess; char *cpustr; size_t ncells = virDomainNumaGetNodeCount(def); size_t i; @@ -827,7 +828,7 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, virBufferAddLit(buf, " unit='KiB'"); if (memAccess) virBufferAsprintf(buf, " memAccess='%s'", - virNumaMemAccessTypeToString(memAccess)); + virDomainMemoryAccessTypeToString(memAccess)); virBufferAddLit(buf, "/>\n"); VIR_FREE(cpustr); } @@ -936,7 +937,7 @@ virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, } -virNumaMemAccess +virDomainMemoryAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa, size_t node) { diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 90deacb..05529ba 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -46,14 +46,14 @@ VIR_ENUM_DECL(virDomainNumatunePlacement) VIR_ENUM_DECL(virDomainNumatuneMemMode) typedef enum { - VIR_NUMA_MEM_ACCESS_DEFAULT, - VIR_NUMA_MEM_ACCESS_SHARED, - VIR_NUMA_MEM_ACCESS_PRIVATE, + VIR_DOMAIN_MEMORY_ACCESS_DEFAULT = 0, /* No memory access defined */ + VIR_DOMAIN_MEMORY_ACCESS_SHARED, /* Memory access is set as shared */ + VIR_DOMAIN_MEMORY_ACCESS_PRIVATE, /* Memory access is set as private */ - VIR_NUMA_MEM_ACCESS_LAST -} virNumaMemAccess; + VIR_DOMAIN_MEMORY_ACCESS_LAST, +} virDomainMemoryAccess; +VIR_ENUM_DECL(virDomainMemoryAccess) -VIR_ENUM_DECL(virNumaMemAccess) virDomainNumaPtr virDomainNumaNew(void); void virDomainNumaFree(virDomainNumaPtr numa); @@ -90,7 +90,7 @@ size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa) virBitmapPtr virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, size_t node) ATTRIBUTE_NONNULL(1); -virNumaMemAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa, +virDomainMemoryAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa, size_t node) ATTRIBUTE_NONNULL(1); unsigned long long virDomainNumaGetNodeMemorySize(virDomainNumaPtr numa, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8e48d2..7d186d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3233,7 +3233,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; const long system_page_size = virGetSystemPageSizeKB(); - virNumaMemAccess memAccess = VIR_NUMA_MEM_ACCESS_DEFAULT; + virDomainMemoryAccess memAccess = VIR_DOMAIN_MEMORY_ACCESS_DEFAULT; size_t i; char *mem_path = NULL; virBitmapPtr nodemask = NULL; @@ -3328,18 +3328,18 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; switch (memAccess) { - case VIR_NUMA_MEM_ACCESS_SHARED: + case VIR_DOMAIN_MEMORY_ACCESS_SHARED: if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) goto cleanup; break; - case VIR_NUMA_MEM_ACCESS_PRIVATE: + case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE: if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0) goto cleanup; break; - case VIR_NUMA_MEM_ACCESS_DEFAULT: - case VIR_NUMA_MEM_ACCESS_LAST: + case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT: + case VIR_DOMAIN_MEMORY_ACCESS_LAST: break; } } else { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e5b77d6..ab67773 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4488,7 +4488,7 @@ qemuProcessStartWarnShmem(virDomainObjPtr vm) if (!shmem && vm->def->mem.nhugepages) { for (i = 0; i < virDomainNumaGetNodeCount(vm->def->numa); i++) { if (virDomainNumaGetNodeMemoryAccessMode(vm->def->numa, i) == - VIR_NUMA_MEM_ACCESS_SHARED) { + VIR_DOMAIN_MEMORY_ACCESS_SHARED) { shmem = true; break; } -- 2.7.4 -------------------------------------------------------------- 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.
On 13.12.2016 13:12, Jaroslav Safka wrote:
Rename to avoid duplicate code. Because virDomainMemoryAccess will be used in memorybacking for setting default behaviour.
NOTE: The enum cannot be moved to qemu/domain_conf because of headers dependency --- src/conf/numa_conf.c | 15 ++++++++------- src/conf/numa_conf.h | 14 +++++++------- src/qemu/qemu_command.c | 10 +++++----- src/qemu/qemu_process.c | 2 +- 4 files changed, 21 insertions(+), 20 deletions(-)
ACK Michal
This first change introduces new xml elements for file based memorybacking support and their parsing. It allows vhost-user to be used without hugepages. New xml elements: <memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking> --- docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 30 +++++ src/conf/domain_conf.c | 133 ++++++++++++++++----- src/conf/domain_conf.h | 22 ++++ .../qemuxml2argv-memorybacking-set.xml | 24 ++++ .../qemuxml2argv-memorybacking-unset.xml | 24 ++++ .../qemuxml2xmlout-memorybacking-set.xml | 32 +++++ .../qemuxml2xmlout-memorybacking-unset.xml | 32 +++++ tests/qemuxml2xmltest.c | 3 + 9 files changed, 276 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9218eec..fc194bf 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -903,6 +903,9 @@ </hugepages> <nosharepages/> <locked/> + <source type="file|anonymous"/> + <access mode="shared|private"/> + <allocation mode="immediate|ondemand"/> </memoryBacking> ... </domain> @@ -942,6 +945,12 @@ most of the host's memory). Doing so may be dangerous to both the domain and the host itself since the host's kernel may run out of memory. <span class="since">Since 1.0.6</span></dd> + <dt><code>source</code></dt> + <dd>In this attribute you can switch to file memorybacking or keep default anonymous.</dd> + <dt><code>access</code></dt> + <dd>Specify if memory is shared or private. This can be overridden per numa node by <code>memAccess</code></dd> + <dt><code>allocation</code></dt> + <dd>Specify when allocate the memory</dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c004233..363ee2f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -560,6 +560,36 @@ <empty/> </element> </optional> + <optional> + <element name="source"> + <attribute name="type"> + <choice> + <value>file</value> + <value>anonymous</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="access"> + <attribute name="mode"> + <choice> + <value>shared</value> + <value>private</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="allocation"> + <attribute name="mode"> + <choice> + <value>immediate</value> + <value>ondemand</value> + </choice> + </attribute> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0bd38d..c54fc97 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -837,6 +837,16 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "abort", "pivot") +VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST, + "none", + "file", + "anonymous") + +VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, + "none", + "immediate", + "ondemand") + VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, "rom", @@ -16528,48 +16538,93 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(tmp); - if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract hugepages nodes")); - goto error; + tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt); + if (tmp) { + if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown memoryBacking/source/type '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); } - if (n) { - if (VIR_ALLOC_N(def->mem.hugepages, n) < 0) + tmp = virXPathString("string(./memoryBacking/access/@mode)", ctxt); + if (tmp) { + if ((def->mem.access = virDomainMemoryAccessTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown memoryBacking/access/mode '%s'"), tmp); goto error; + } + VIR_FREE(tmp); + } - for (i = 0; i < n; i++) { - if (virDomainHugepagesParseXML(nodes[i], ctxt, - &def->mem.hugepages[i]) < 0) + tmp = virXPathString("string(./memoryBacking/allocation/@mode)", ctxt); + if (tmp) { + if ((def->mem.allocation = virDomainMemoryAllocationTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown memoryBacking/allocation/mode '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); + } + + if (virXPathNode("./memoryBacking/hugepages", ctxt)) { + /* hugepages will be used */ + + if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with memory allocation ondemand")); + goto error; + } + + if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with anonymous memory source")); + goto error; + } + + if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot extract hugepages nodes")); + goto error; + } + + if (n) { + if (VIR_ALLOC_N(def->mem.hugepages, n) < 0) goto error; - def->mem.nhugepages++; - for (j = 0; j < i; j++) { - if (def->mem.hugepages[i].nodemask && - def->mem.hugepages[j].nodemask && - virBitmapOverlaps(def->mem.hugepages[i].nodemask, - def->mem.hugepages[j].nodemask)) { - virReportError(VIR_ERR_XML_DETAIL, - _("nodeset attribute of hugepages " - "of sizes %llu and %llu intersect"), - def->mem.hugepages[i].size, - def->mem.hugepages[j].size); - goto error; - } else if (!def->mem.hugepages[i].nodemask && - !def->mem.hugepages[j].nodemask) { - virReportError(VIR_ERR_XML_DETAIL, - _("two master hugepages detected: " - "%llu and %llu"), - def->mem.hugepages[i].size, - def->mem.hugepages[j].size); + for (i = 0; i < n; i++) { + if (virDomainHugepagesParseXML(nodes[i], ctxt, + &def->mem.hugepages[i]) < 0) goto error; + def->mem.nhugepages++; + + for (j = 0; j < i; j++) { + if (def->mem.hugepages[i].nodemask && + def->mem.hugepages[j].nodemask && + virBitmapOverlaps(def->mem.hugepages[i].nodemask, + def->mem.hugepages[j].nodemask)) { + virReportError(VIR_ERR_XML_DETAIL, + _("nodeset attribute of hugepages " + "of sizes %llu and %llu intersect"), + def->mem.hugepages[i].size, + def->mem.hugepages[j].size); + goto error; + } else if (!def->mem.hugepages[i].nodemask && + !def->mem.hugepages[j].nodemask) { + virReportError(VIR_ERR_XML_DETAIL, + _("two master hugepages detected: " + "%llu and %llu"), + def->mem.hugepages[i].size, + def->mem.hugepages[j].size); + goto error; + } } } - } - VIR_FREE(nodes); - } else { - if ((node = virXPathNode("./memoryBacking/hugepages", ctxt))) { + VIR_FREE(nodes); + } else { + /* no hugepage pages */ if (VIR_ALLOC(def->mem.hugepages) < 0) goto error; @@ -23658,7 +23713,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</memtune>\n"); } - if (def->mem.nhugepages || def->mem.nosharepages || def->mem.locked) { + if (def->mem.nhugepages || def->mem.nosharepages || def->mem.locked + || def->mem.source || def->mem.access || def->mem.allocation) + { virBufferAddLit(buf, "<memoryBacking>\n"); virBufferAdjustIndent(buf, 2); if (def->mem.nhugepages) @@ -23667,6 +23724,16 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "<nosharepages/>\n"); if (def->mem.locked) virBufferAddLit(buf, "<locked/>\n"); + if (def->mem.source) + virBufferAsprintf(buf, "<source type='%s'/>\n", + virDomainMemorySourceTypeToString(def->mem.source)); + if (def->mem.access) + virBufferAsprintf(buf, "<access mode='%s'/>\n", + virDomainMemoryAccessTypeToString(def->mem.access)); + if (def->mem.allocation) + virBufferAsprintf(buf, "<allocation mode='%s'/>\n", + virDomainMemoryAllocationTypeToString(def->mem.allocation)); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</memoryBacking>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 114e506..d5b679a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -586,6 +586,22 @@ typedef enum { VIR_DOMAIN_DISK_MIRROR_STATE_LAST } virDomainDiskMirrorState; +typedef enum { + VIR_DOMAIN_MEMORY_SOURCE_NONE = 0, /* No memory source defined */ + VIR_DOMAIN_MEMORY_SOURCE_FILE, /* Memory source is set as file */ + VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS, /* Memory source is set as anonymous */ + + VIR_DOMAIN_MEMORY_SOURCE_LAST, +} virDomainMemorySource; + +typedef enum { + VIR_DOMAIN_MEMORY_ALLOCATION_NONE = 0, /* No memory allocation defined */ + VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE, /* Memory allocation is set as immediate */ + VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND, /* Memory allocation is set as ondemand */ + + VIR_DOMAIN_MEMORY_ALLOCATION_LAST, +} virDomainMemoryAllocation; + /* Stores the virtual disk configuration */ struct _virDomainDiskDef { @@ -2129,6 +2145,10 @@ struct _virDomainMemtune { unsigned long long soft_limit; /* in kibibytes, limit at off_t bytes */ unsigned long long min_guarantee; /* in kibibytes, limit at off_t bytes */ unsigned long long swap_hard_limit; /* in kibibytes, limit at off_t bytes */ + + virDomainMemorySource source; /* enum virDomainMemorySource */ + virDomainMemoryAccess access; /* enum virDomainMemoryAccess */ + virDomainMemoryAllocation allocation; /* enum virDomainMemoryAllocation */ }; typedef struct _virDomainPowerManagement virDomainPowerManagement; @@ -3114,6 +3134,8 @@ VIR_ENUM_DECL(virDomainTPMModel) VIR_ENUM_DECL(virDomainTPMBackend) VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) +VIR_ENUM_DECL(virDomainMemorySource) +VIR_ENUM_DECL(virDomainMemoryAllocation) VIR_ENUM_DECL(virDomainIOMMUModel) VIR_ENUM_DECL(virDomainShmemModel) /* from libvirt.h */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml new file mode 100644 index 0000000..e23bc36 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>SomeDummyGuest</name> + <uuid>ef1bdff4-27f3-4e85-a807-5fb4d58463cc</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='shared'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml new file mode 100644 index 0000000..6f50765 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>SomeDummyGuest</name> + <uuid>ef1bdff4-27f3-4e85-a807-5fb4d58463cc</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <source type="anonymous"/> + <access mode="private"/> + <allocation mode="ondemand"/> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml new file mode 100644 index 0000000..fb07472 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>SomeDummyGuest</name> + <uuid>ef1bdff4-27f3-4e85-a807-5fb4d58463cc</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='shared'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml new file mode 100644 index 0000000..ac2a278 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>SomeDummyGuest</name> + <uuid>ef1bdff4-27f3-4e85-a807-5fb4d58463cc</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <source type='anonymous'/> + <access mode='private'/> + <allocation mode='ondemand'/> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ddd17cb..e4d011f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1013,6 +1013,9 @@ mymain(void) DO_TEST("virtio-input", NONE); DO_TEST("virtio-input-passthrough", NONE); + DO_TEST("memorybacking-set", NONE); + DO_TEST("memorybacking-unset", NONE); + virObjectUnref(cfg); DO_TEST("acpi-table", NONE); -- 2.7.4 -------------------------------------------------------------- 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.
On 13.12.2016 13:12, Jaroslav Safka wrote:
This first change introduces new xml elements for file based memorybacking support and their parsing. It allows vhost-user to be used without hugepages.
New xml elements: <memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking> --- docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 30 +++++ src/conf/domain_conf.c | 133 ++++++++++++++++----- src/conf/domain_conf.h | 22 ++++ .../qemuxml2argv-memorybacking-set.xml | 24 ++++ .../qemuxml2argv-memorybacking-unset.xml | 24 ++++ .../qemuxml2xmlout-memorybacking-set.xml | 32 +++++ .../qemuxml2xmlout-memorybacking-unset.xml | 32 +++++ tests/qemuxml2xmltest.c | 3 + 9 files changed, 276 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9218eec..fc194bf 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -903,6 +903,9 @@ </hugepages> <nosharepages/> <locked/> + <source type="file|anonymous"/> + <access mode="shared|private"/> + <allocation mode="immediate|ondemand"/> </memoryBacking> ... </domain> @@ -942,6 +945,12 @@ most of the host's memory). Doing so may be dangerous to both the domain and the host itself since the host's kernel may run out of memory. <span class="since">Since 1.0.6</span></dd> + <dt><code>source</code></dt> + <dd>In this attribute you can switch to file memorybacking or keep default anonymous.</dd> + <dt><code>access</code></dt> + <dd>Specify if memory is shared or private. This can be overridden per numa node by <code>memAccess</code></dd> + <dt><code>allocation</code></dt> + <dd>Specify when allocate the memory</dd> </dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c004233..363ee2f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -560,6 +560,36 @@ <empty/> </element> </optional> + <optional> + <element name="source"> + <attribute name="type"> + <choice> + <value>file</value> + <value>anonymous</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="access"> + <attribute name="mode"> + <choice> + <value>shared</value> + <value>private</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="allocation"> + <attribute name="mode"> + <choice> + <value>immediate</value> + <value>ondemand</value> + </choice> + </attribute> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0bd38d..c54fc97 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -837,6 +837,16 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "abort", "pivot")
+VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST, + "none", + "file", + "anonymous") + +VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, + "none", + "immediate", + "ondemand") + VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, "rom", @@ -16528,48 +16538,93 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(tmp);
- if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract hugepages nodes")); - goto error; + tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt); + if (tmp) { + if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) < 0) {
This will not fly. def->mem.source is typeof enum virDomainMemorySource which can have values 0, 1 or 2. Now, should virDomainMemorySourceTypeFromString() return an error (-1) (because of unknown @type value), this gets squeezed into the enum range, which always falls into 0-2 range. IOW, this condition will never be true. That's why we have all those: int blah; /* enum virMyAwesomeEnum */ lines in the definition. Had the @source been an int, this starts working. Except, we might want to disallow "none" and thus the condition should be "<= 0". This also apply for the other two variables.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown memoryBacking/source/type '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); }
ACK with that change. Michal
Comments inside.
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com Cc: Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 2/3] conf: Add new xml elements for file memorybacking support
On 13.12.2016 13:12, Jaroslav Safka wrote:
This first change introduces new xml elements for file based memorybacking support and their parsing. It allows vhost-user to be used without hugepages.
New xml elements: <memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking> --- docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 30 +++++ src/conf/domain_conf.c | 133 ++++++++++++++++----- src/conf/domain_conf.h | 22 ++++ .../qemuxml2argv-memorybacking-set.xml | 24 ++++ .../qemuxml2argv-memorybacking-unset.xml | 24 ++++ .../qemuxml2xmlout-memorybacking-set.xml | 32 +++++ .../qemuxml2xmlout-memorybacking-unset.xml | 32 +++++ tests/qemuxml2xmltest.c | 3 + 9 files changed, 276 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9218eec..fc194bf 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -903,6 +903,9 @@ </hugepages> <nosharepages/> <locked/> + <source type="file|anonymous"/> + <access mode="shared|private"/> + <allocation mode="immediate|ondemand"/> </memoryBacking> ... </domain> @@ -942,6 +945,12 @@ most of the host's memory). Doing so may be dangerous to both the domain and the host itself since the host's kernel may run out of memory. <span class="since">Since 1.0.6</span></dd> + <dt><code>source</code></dt> + <dd>In this attribute you can switch to file memorybacking or keep default anonymous.</dd> + <dt><code>access</code></dt> + <dd>Specify if memory is shared or private. This can be overridden per numa node by <code>memAccess</code></dd> + <dt><code>allocation</code></dt> + <dd>Specify when allocate the memory</dd> </dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c004233..363ee2f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -560,6 +560,36 @@ <empty/> </element> </optional> + <optional> + <element name="source"> + <attribute name="type"> + <choice> + <value>file</value> + <value>anonymous</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="access"> + <attribute name="mode"> + <choice> + <value>shared</value> + <value>private</value> + </choice> + </attribute> + </element> + </optional> + <optional> + <element name="allocation"> + <attribute name="mode"> + <choice> + <value>immediate</value> + <value>ondemand</value> + </choice> + </attribute> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0bd38d..c54fc97 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -837,6 +837,16 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "abort", "pivot")
+VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST, + "none", + "file", + "anonymous") + +VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, + "none", + "immediate", + "ondemand") + VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, "rom", @@ -16528,48 +16538,93 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(tmp);
- if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract hugepages nodes")); - goto error; + tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt); + if (tmp) { + if ((def->mem.source = + virDomainMemorySourceTypeFromString(tmp)) < 0) {
This will not fly. def->mem.source is typeof enum virDomainMemorySource which can have values 0, 1 or 2. Now, should virDomainMemorySourceTypeFromString() return an error (-1) (because of unknown @type value), this gets squeezed into the enum range, which always falls into 0-2 range. IOW, this condition will never be true. That's why we have all those:
int blah; /* enum virMyAwesomeEnum */
lines in the definition. Had the @source been an int, this starts working. Except, we might want to disallow "none" and thus the condition should be "<= 0". This also apply for the other two variables.
[Jarek] I see. I saw using enums in another structure, then I changed it but I will return it to int. Thanks for point it out
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown memoryBacking/source/type '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); }
ACK with that change.
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.
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 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, 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; - } + 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"; + } } 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; } @@ -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/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args new file mode 100644 index 0000000..951a7cb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name instance-00000092 \ +-S \ +-M pc-i440fx-wily \ +-m 14336 \ +-mem-prealloc \ +-smp 8,sockets=8,cores=1,threads=1 \ +-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-instance-00000092/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml new file mode 100644 index 0000000..243b65d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml @@ -0,0 +1,27 @@ +<domain type='kvm' id='56'> + <name>instance-00000092</name> + <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid> + <memory unit='KiB'>14680064</memory> + <currentMemory unit='KiB'>14680064</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='shared'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-wily'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='8' cores='1' threads='1'/> + </cpu> + <clock offset='utc' /> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args new file mode 100644 index 0000000..027e290 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name instance-00000092 \ +-S \ +-M pc-i440fx-wily \ +-m 14336 \ +-mem-prealloc \ +-smp 8,sockets=1,cores=8,threads=1 \ +-object memory-backend-file,id=ram-node0,mem-path=/tmp/lib,share=yes,\ +size=15032385536 \ +-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \ +-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-instance-00000092/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml new file mode 100644 index 0000000..ca5402d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml @@ -0,0 +1,30 @@ +<domain type='kvm'> + <name>instance-00000092</name> + <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid> + <memory unit='KiB'>14680064</memory> + <currentMemory unit='KiB'>14680064</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='shared'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-wily'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='1' cores='8' threads='1'/> + <numa> + <cell id='0' cpus='0-7' memory='14680064' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc' /> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args new file mode 100644 index 0000000..396867d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name instance-00000092 \ +-S \ +-M pc-i440fx-wily \ +-m 28672 \ +-mem-prealloc \ +-smp 20,sockets=1,cores=8,threads=1 \ +-object memory-backend-file,id=ram-node0,mem-path=/tmp/lib,size=15032385536 \ +-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \ +-object memory-backend-file,id=ram-node1,mem-path=/tmp/lib,share=yes,\ +size=15032385536 \ +-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \ +-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-instance-00000092/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.xml b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.xml new file mode 100644 index 0000000..49beff1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.xml @@ -0,0 +1,31 @@ + <domain type='kvm' id='56'> + <name>instance-00000092</name> + <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid> + <memory unit='KiB'>14680064</memory> + <currentMemory unit='KiB'>14680064</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='private'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>20</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-wily'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='1' cores='8' threads='1'/> + <numa> + <cell id='0' cpus='0-7' memory='14680064' unit='KiB'/> + <cell id='1' cpus='8-15' memory='14680064' unit='KiB' memAccess='shared'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='virtio'/> + </devices> + </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args new file mode 100644 index 0000000..bfd91a8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name instance-00000092 \ +-S \ +-M pc-i440fx-wily \ +-m 43008 \ +-mem-prealloc \ +-smp 32,sockets=1,cores=24,threads=1 \ +-object memory-backend-file,id=ram-node0,mem-path=/tmp/lib,share=yes,\ +size=15032385536 \ +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-object memory-backend-file,id=ram-node1,mem-path=/tmp/lib,share=yes,\ +size=15032385536 \ +-numa node,nodeid=1,cpus=2-3,memdev=ram-node1 \ +-object memory-backend-file,id=ram-node2,mem-path=/tmp/lib,share=no,\ +size=15032385536 \ +-numa node,nodeid=2,cpus=4-5,memdev=ram-node2 \ +-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-instance-00000092/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.xml b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.xml new file mode 100644 index 0000000..7933507 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.xml @@ -0,0 +1,32 @@ + <domain type='kvm' id='56'> + <name>instance-00000092</name> + <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid> + <memory unit='KiB'>14680064</memory> + <currentMemory unit='KiB'>14680064</currentMemory> + <memoryBacking> + <source type='file'/> + <access mode='shared'/> + <allocation mode='immediate'/> + </memoryBacking> + <vcpu placement='static'>32</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-wily'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='1' cores='24' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='14680064' unit='KiB'/> + <cell id='1' cpus='2-3' memory='14680064' unit='KiB' memAccess='shared'/> + <cell id='2' cpus='4-5' memory='14680064' unit='KiB' memAccess='private'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='virtio'/> + </devices> + </domain> 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); 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); DO_TEST("numatune-auto-prefer", NONE); DO_TEST("numatune-memnode", NONE); -- 2.7.4 -------------------------------------------------------------- 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.
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
Comments inside
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com Cc: Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> 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?
[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.
On Sat, Jan 28, 2017 at 03:03:16PM +0100, Michal Privoznik wrote:
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?
You don't want multi-GB files created in /tmp, nor cfg->stateDir which is also on tmpfs Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:38 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 3/3] qemu: Add args generation for file memory backing
On Sat, Jan 28, 2017 at 03:03:16PM +0100, Michal Privoznik wrote:
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?
You don't want multi-GB files created in /tmp, nor cfg->stateDir which is also on tmpfs
[Jarek] Hi Daniel, then should I keep the libDir? Or what will be better?
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
-------------------------------------------------------------- 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.
On Mon, Jan 30, 2017 at 11:38:08AM +0000, Daniel P. Berrange wrote:
On Sat, Jan 28, 2017 at 03:03:16PM +0100, Michal Privoznik wrote:
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?
You don't want multi-GB files created in /tmp, nor cfg->stateDir which is also on tmpfs
We should probably add a new option into qemu.conf which will configure the path where those files will be stored. It's not user friendly if you cannot change the default path. Pavel
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>? I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans) b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious. I have the patches applied locally with all the changes I've pointed out applied. If we have answers to both of the questions, I can push these. Michal
Hi, first , thanks much for review. Comments inside
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com Cc: Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com>; Daniel P. Berrange <berrange@redhat.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,s hare=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>? [Jarek] disagree with removing the test or with xml change?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans) b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
[Jarek] is there any link or keywords for find to these previous discussions? I'm not able to find it :(
I have the patches applied locally with all the changes I've pointed out applied. If we have answers to both of the questions, I can push these.
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.
On 01/30/2017 09:07 AM, Safka, JaroslavX wrote:
Hi, first , thanks much for review. Comments inside
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com Cc: Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com>; Daniel P. Berrange <berrange@redhat.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,s hare=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>? [Jarek] disagree with removing the test or with xml change?
XML change is okay. But I don't think that behaviour is changed (rather than extended). I mean, prior to these patches the following configuration errors out: <cpu> <numa> <cell id='0' cpus='0-3' memory='4194304' unit='KiB' memAccess='shared'/> </numa> </cpu> With your patches, the error is no longer observed. Wait, is this expected? If so, than you need to remove that check in qemuBuildMemoryBackendStr in 3/3 which I told you not to do and also: diff --git i/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c index f09c5460f..feb50f7a2 100644 --- i/tests/qemuxml2argvtest.c +++ w/tests/qemuxml2argvtest.c @@ -1516,7 +1516,7 @@ 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("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_RAM); DO_TEST_FAILURE("cpu-numa-memshared", NONE); DO_TEST("cpu-host-model", NONE); DO_TEST("cpu-host-model-vendor", NONE);
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans) b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
[Jarek] is there any link or keywords for find to these previous discussions? I'm not able to find it :(
What discussion do you have in mind? The a) rule was not a written kind of rule.
I have the patches applied locally with all the changes I've pointed out applied. If we have answers to both of the questions, I can push these.
This is still true. So no need to rework the patches on your side (you can view this as my attempt to say sorry for such late review). Michal
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote:
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt is reasonable. Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt is reasonable. Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
-------------------------------------------------------------- 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.
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt is reasonable. Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path. Pavel
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt is reasonable. Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be
configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
Pavel
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.
On Mon, Jan 30, 2017 at 12:16:38PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote:
Hi, we would like to introduce 3 new elements source,access and allocation in memoryBacking element. For now it was made for numa topology.
<memoryBacking> <source type="file|anonymous"/> <access mode="shared|private"/> <allocation mode="immediate|ondemand"/> </memoryBacking>
If allocation is immediate then -mem-prealloc should be added to the qemu commanline. If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path* -numa node,memdev=mem Will be added to the qemu commandline
If access is shared then the "share=on" parameter will be added to the memory-backend-file e.g. -object memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
,share=on
The access mode can be overriden by specifying token memAccess in numa cell.
The test cpu-numa-memshared was removed, because behaviour was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt is reasonable. Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be
configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
It's not that hard :) for example this commit 661887f5582 can be used as an example how to add a new configurable option into qemu.conf. We have whole month before next release so it would be nice to have it as part of the same patch series. Pavel
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:50 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:16:38PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote: > Hi, > we would like to introduce 3 new elements source,access and > allocation in memoryBacking element. > For now it was made for numa topology. > > <memoryBacking> > <source type="file|anonymous"/> > <access mode="shared|private"/> > <allocation mode="immediate|ondemand"/> > </memoryBacking> > > If allocation is immediate then -mem-prealloc should be > added to the qemu commanline. > If source is file then > -object memory-backend-file,id=mem,size=1024M,mem-path=*lib > dir > path* -numa node,memdev=mem Will be added to the qemu > commandline > > If access is shared then the "share=on" parameter will be > added to the memory-backend-file e.g. > -object > memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
> ,share=on > > The access mode can be overriden by specifying token > memAccess in numa cell. > > The test cpu-numa-memshared was removed, because behaviour > was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt
is reasonable.
Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
It's not that hard :) for example this commit 661887f5582 can be used as an example how to add a new configurable option into qemu.conf. We have whole month before next release so it would be nice to have it as part of the same patch series.
Pavel
[Jarek] Thanks! Will be ok to use config parameter name "file_backing_memory_path"? Or you suggest different name? -------------------------------------------------------------- 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.
On Tue, Jan 31, 2017 at 09:09:25AM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:50 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:16:38PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: > On 13.12.2016 13:12, Jaroslav Safka wrote: > > Hi, > > we would like to introduce 3 new elements source,access and > > allocation in memoryBacking element. > > For now it was made for numa topology. > > > > <memoryBacking> > > <source type="file|anonymous"/> > > <access mode="shared|private"/> > > <allocation mode="immediate|ondemand"/> > > </memoryBacking> > > > > If allocation is immediate then -mem-prealloc should be > > added to the qemu commanline. > > If source is file then > > -object memory-backend-file,id=mem,size=1024M,mem-path=*lib > > dir > > path* -numa node,memdev=mem Will be added to the qemu > > commandline > > > > If access is shared then the "share=on" parameter will be > > added to the memory-backend-file e.g. > > -object > > memory-backend-file,id=mem,size=1024M,mem- path=/var/lib/libvirt/qemu > > ,share=on > > > > The access mode can be overriden by specifying token > > memAccess in numa cell. > > > > The test cpu-numa-memshared was removed, because behaviour > > was changed and is not needed anymore > > I beg to disagree. What if you don't have any <memoryBacking/>? > > I like these patches, but I'm not certainly sure about: > a) domain XML (in the past we used to require an ACK on schema > change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
> b) the location for qemu to create its mmaped files (patch 3/3). > cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt
is reasonable.
Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
It's not that hard :) for example this commit 661887f5582 can be used as an example how to add a new configurable option into qemu.conf. We have whole month before next release so it would be nice to have it as part of the same patch series.
Pavel
[Jarek] Thanks! Will be ok to use config parameter name "file_backing_memory_path"? Or you suggest different name?
I would recommend "memory_backing_file_dir" and the reason is that we usually start with the most generic description and continue with more and more specific description and our configure options uses mostly "dir" instead of "path". About the location of this configure option I would put it after "hugetlbfs_mount" because it also configure memory backing. Pavel
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:50 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:16:38PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On 13.12.2016 13:12, Jaroslav Safka wrote: > Hi, > we would like to introduce 3 new elements source,access and > allocation in memoryBacking element. > For now it was made for numa topology. > > <memoryBacking> > <source type="file|anonymous"/> > <access mode="shared|private"/> > <allocation mode="immediate|ondemand"/> > </memoryBacking> > > If allocation is immediate then -mem-prealloc should be > added to the qemu commanline. > If source is file then > -object memory-backend-file,id=mem,size=1024M,mem-path=*lib > dir > path* -numa node,memdev=mem Will be added to the qemu > commandline > > If access is shared then the "share=on" parameter will be > added to the memory-backend-file e.g. > -object > memory-backend-file,id=mem,size=1024M,mem-
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: path=/var/lib/libvirt/qemu
> ,share=on > > The access mode can be overriden by specifying token > memAccess in numa cell. > > The test cpu-numa-memshared was removed, because behaviour > was changed and is not needed anymore
I beg to disagree. What if you don't have any <memoryBacking/>?
I like these patches, but I'm not certainly sure about: a) domain XML (in the past we used to require an ACK on schema change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
b) the location for qemu to create its mmaped files (patch 3/3). cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt
is reasonable.
Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
It's not that hard :) for example this commit 661887f5582 can be used as an example how to add a new configurable option into qemu.conf. We have whole month before next release so it would be nice to have it as part of the same patch series.
Pavel
[Jarek] Also I'm not sure where the config parameter put. Probably in the "process_entry" ? -------------------------------------------------------------- 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.
On 01/31/2017 10:40 AM, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:50 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:16:38PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Pavel Hrdina [mailto:phrdina@redhat.com] Sent: Monday, January 30, 2017 1:13 PM To: Safka, JaroslavX <jaroslavx.safka@intel.com> Cc: Daniel P. Berrange <berrange@redhat.com>; Michal Privoznik <mprivozn@redhat.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Mon, Jan 30, 2017 at 12:06:19PM +0000, Safka, JaroslavX wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, January 30, 2017 12:35 PM To: Michal Privoznik <mprivozn@redhat.com> Cc: Safka, JaroslavX <jaroslavx.safka@intel.com>; libvir-list@redhat.com; Mooney, Sean K <sean.k.mooney@intel.com>; Ptacek, MichalX <michalx.ptacek@intel.com> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
On Sat, Jan 28, 2017 at 03:03:02PM +0100, Michal Privoznik wrote: > On 13.12.2016 13:12, Jaroslav Safka wrote: >> Hi, >> we would like to introduce 3 new elements source,access and >> allocation in memoryBacking element. >> For now it was made for numa topology. >> >> <memoryBacking> >> <source type="file|anonymous"/> >> <access mode="shared|private"/> >> <allocation mode="immediate|ondemand"/> >> </memoryBacking> >> >> If allocation is immediate then -mem-prealloc should be >> added to the qemu commanline. >> If source is file then >> -object memory-backend-file,id=mem,size=1024M,mem-path=*lib >> dir >> path* -numa node,memdev=mem Will be added to the qemu >> commandline >> >> If access is shared then the "share=on" parameter will be >> added to the memory-backend-file e.g. >> -object >> memory-backend-file,id=mem,size=1024M,mem- path=/var/lib/libvirt/qemu >> ,share=on >> >> The access mode can be overriden by specifying token >> memAccess in numa cell. >> >> The test cpu-numa-memshared was removed, because behaviour >> was changed and is not needed anymore > > I beg to disagree. What if you don't have any <memoryBacking/>? > > I like these patches, but I'm not certainly sure about: > a) domain XML (in the past we used to require an ACK on schema > change from one of the Dans)
What are you unsure about ? This XML is what I suggested in previous rounds of discussion.
> b) the location for qemu to create its mmaped files (patch 3/3). > cfg->libDir looks very suspicious.
Well we've got two possibilities - source=anonymous, should be using /dev/shm, similar to how we do huge pages. For source=file, we need a real filesystem. Something under /var/lib/libvirt
is reasonable.
Perhaps a dedicated dir is needed ? eg /var/lib/libvirt/ram in order to provide a point where the admin can mount a sufficiently large filesystem ? Or make it configurable in /etc/libvirt/qemu.conf
[Jarek] It will be ok when I do something like libDir + "/ram". Or It should be configurable on compilation time?
Oh, I see that Dan already suggested to make it configurable. I would say both. Make it configurable in qemu.conf and use /var/lib/libvirt/ram as a default path.
[Jarek] will be ok to make it configurable in another patch set? Or it should be all in one patch set? Because it will take me some time, when I find out how to add this parameter as configurable.
It's not that hard :) for example this commit 661887f5582 can be used as an example how to add a new configurable option into qemu.conf. We have whole month before next release so it would be nice to have it as part of the same patch series.
Pavel
[Jarek] Also I'm not sure where the config parameter put. Probably in the "process_entry" ?
I'd go with memory_backing_path and create new memory_entry. Michal
participants (5)
-
Daniel P. Berrange -
Jaroslav Safka -
Michal Privoznik -
Pavel Hrdina -
Safka, JaroslavX