[libvirt] [PATCH 0/2] Cleanup memoryBacking docs and move some hugepages checks
While reviewing the -memfd series: https://www.redhat.com/archives/libvir-list/2018-September/msg00381.html I noted that a couple of checks seemed out of place in the the domain_conf processing. They seem more "in place" in the hypvervisor processing. And while doing this I also noted the documentation is clearly not up to snuff, so I modified that too. That's a followup to another recent review comment: https://www.redhat.com/archives/libvir-list/2018-September/msg00176.html although I didn't delve into much detail, just the basics of what the attributes are and the acceptible values. John Ferlan (2): doc: Update the wording around the backingStore conf: Move hypervisor specific nhugepage checks docs/formatdomain.html.in | 13 ++++++++----- src/conf/domain_conf.c | 14 -------------- src/qemu/qemu_domain.c | 27 ++++++++++++++++++++++----- 3 files changed, 30 insertions(+), 24 deletions(-) -- 2.17.1
Commit bc6d3121a was far too terse when describing the new elements, attributes, and allow values. Provide a few more words to help describe. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb619a1656..1f12ab5b42 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1150,13 +1150,16 @@ suitable for the specific environment at the same time to mitigate the risks described above. <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> + <dd>Using the <code>type</code> attribute, it's possible to provide + "file" to utilize file memorybacking or keep the 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> + <dd>Using the <code>mode</code> attribute, specify if the memory is + to be "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> + <dd>Using the <code>mode</code> attribute, specify when to allocate + the memory by supplying either "immediate" or "ondemand".</dd> <dt><code>discard</code></dt> <dd>When set and supported by hypervisor the memory content is discarded just before guest shuts down (or -- 2.17.1
On Tue, Sep 11, 2018 at 5:36 PM John Ferlan <jferlan@redhat.com> wrote:
Commit bc6d3121a was far too terse when describing the new elements, attributes, and allow values. Provide a few more words to help describe.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--- docs/formatdomain.html.in | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb619a1656..1f12ab5b42 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1150,13 +1150,16 @@ suitable for the specific environment at the same time to mitigate the risks described above. <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> + <dd>Using the <code>type</code> attribute, it's possible to provide + "file" to utilize file memorybacking or keep the 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> + <dd>Using the <code>mode</code> attribute, specify if the memory is + to be "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> + <dd>Using the <code>mode</code> attribute, specify when to allocate + the memory by supplying either "immediate" or "ondemand".</dd> <dt><code>discard</code></dt> <dd>When set and supported by hypervisor the memory content is discarded just before guest shuts down (or -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
Commit 82327038 moved a couple of checks out of the XML parser into the domain validation; however, those checks seem to be more useful as hypervisor specific checks rather than the more general domain conf checks (nothing in the docs indicate a specific error). Fortunately only QEMU was processing the memoryBacking, thus add the changes to qemuDomainDefValidateMemory and change the code a bit to make usage of the similar deref to def->mem and the mem->nhugepages filter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 14 -------------- src/qemu/qemu_domain.c | 27 ++++++++++++++++++++++----- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7e14cea128..cbc3497c47 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6182,20 +6182,6 @@ virDomainDefMemtuneValidate(const virDomainDef *def) if (mem->nhugepages == 0) return 0; - if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("hugepages are not allowed with memory " - "allocation ondemand")); - return -1; - } - - if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("hugepages are not allowed with anonymous " - "memory source")); - return -1; - } - for (i = 0; i < mem->nhugepages; i++) { size_t j; ssize_t nextBit; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5329899b13..30fd21dcdf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3953,18 +3953,35 @@ static int qemuDomainDefValidateMemory(const virDomainDef *def) { const long system_page_size = virGetSystemPageSizeKB(); + const virDomainMemtune *mem = &(def->mem); + + if (mem->nhugepages == 0) + return 0; + + if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with memory " + "allocation ondemand")); + return -1; + } + + if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with anonymous " + "memory source")); + return -1; + } /* We can't guarantee any other mem.access * if no guest NUMA nodes are defined. */ - if (def->mem.nhugepages != 0 && - def->mem.hugepages[0].size != system_page_size && + if (mem->hugepages[0].size != system_page_size && virDomainNumaGetNodeCount(def->numa) == 0 && - def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && - def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + mem->access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("memory access mode '%s' not supported " "without guest numa node"), - virDomainMemoryAccessTypeToString(def->mem.access)); + virDomainMemoryAccessTypeToString(mem->access)); return -1; } -- 2.17.1
Hi On Tue, Sep 11, 2018 at 5:36 PM John Ferlan <jferlan@redhat.com> wrote:
Commit 82327038 moved a couple of checks out of the XML parser into the domain validation; however, those checks seem to be more useful as hypervisor specific checks rather than the more general domain conf checks (nothing in the docs indicate a specific error).
Fortunately only QEMU was processing the memoryBacking, thus add the changes to qemuDomainDefValidateMemory and change the code a bit to make usage of the similar deref to def->mem and the mem->nhugepages filter.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--- src/conf/domain_conf.c | 14 -------------- src/qemu/qemu_domain.c | 27 ++++++++++++++++++++++----- 2 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7e14cea128..cbc3497c47 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6182,20 +6182,6 @@ virDomainDefMemtuneValidate(const virDomainDef *def) if (mem->nhugepages == 0) return 0;
That check could be removed now.
- if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("hugepages are not allowed with memory " - "allocation ondemand")); - return -1; - } - - if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("hugepages are not allowed with anonymous " - "memory source")); - return -1; - } - for (i = 0; i < mem->nhugepages; i++) { size_t j; ssize_t nextBit; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5329899b13..30fd21dcdf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3953,18 +3953,35 @@ static int qemuDomainDefValidateMemory(const virDomainDef *def) { const long system_page_size = virGetSystemPageSizeKB(); + const virDomainMemtune *mem = &(def->mem); +
extra parenthesis?
+ if (mem->nhugepages == 0) + return 0; + + if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with memory " + "allocation ondemand")); + return -1; + } + + if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("hugepages are not allowed with anonymous " + "memory source")); + return -1; + }
/* We can't guarantee any other mem.access * if no guest NUMA nodes are defined. */ - if (def->mem.nhugepages != 0 && - def->mem.hugepages[0].size != system_page_size && + if (mem->hugepages[0].size != system_page_size && virDomainNumaGetNodeCount(def->numa) == 0 && - def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && - def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + mem->access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("memory access mode '%s' not supported " "without guest numa node"), - virDomainMemoryAccessTypeToString(def->mem.access)); + virDomainMemoryAccessTypeToString(mem->access)); return -1; }
--
looks good otherwise Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
participants (2)
-
John Ferlan -
Marc-André Lureau