Re: [libvirt] [PATCH 03/10] qemu: command: Make qemuBuildMemoryBackendStr usable without NUMA
On 10/16/2015 08:11 AM, Peter Krempa wrote:
Make the function usable so that -1 can be passed to it as cell ID
so
that we can later enable memory hotplug on non-NUMA guests for certain
architectures.
I've inspected all functions that take guestNode as an argument to
verify that they are eiter safe to be called or are not called at all.
either
Although it seems that comment is left for under the --- ...
---
src/qemu/qemu_command.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f727d0b..a37a4fa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5011,7 +5011,8 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
* qemuBuildMemoryBackendStr:
* @size: size of the memory device in kibibytes
* @pagesize: size of the requested memory page in KiB, 0 for default
- * @guestNode: NUMA node in the guest that the memory object will be attached to
+ * @guestNode: NUMA node in the guest that the memory object will be attached
+ * to, or -1 if NUMA is not used in the guest
* @hostNodes: map of host nodes to alloc the memory in, NULL for default
* @autoNodeset: fallback nodeset in case of automatic numa placement
* @def: domain definition object
@@ -5058,7 +5059,8 @@ qemuBuildMemoryBackendStr(unsigned long long size,
*backendType = NULL;
/* memory devices could provide a invalid guest node */
- if (guestNode >= virDomainNumaGetNodeCount(def->numa)) {
+ if (guestNode >= 0 &&
+ guestNode >= virDomainNumaGetNodeCount(def->numa)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("can't add memory backend for guest node '%d'
as "
"the guest has only '%zu' NUMA nodes
configured"),
@@ -5069,7 +5071,9 @@ qemuBuildMemoryBackendStr(unsigned long long size,
if (!(props = virJSONValueNewObject()))
return -1;
^^ this could move
- memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
+ if (guestNode >= 0)
+ memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
+
if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
virDomainNumatuneGetMode(def->numa, -1, &mode) < 0)
mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
^^
Seems to me the code could be adjusted a bit to have:
if (guestNode >= 0) {
if (guestNode >= virDomainNumaGetNodeCount(def->numa))
...
memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa,
guestNode);
...
if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0
&&
...
}
where 'mode' is initialized to VIR_DOMAIN_NUMATUNE_MEM_STRICT;
and the props call moves either before or after the blob.
@@ -5086,6 +5090,10 @@ qemuBuildMemoryBackendStr(unsigned long long
size,
continue;
}
+ /* just find the master hugepage in case we don't use NUMA */
+ if (guestNode < 0)
+ continue;
+
So what you're say is if master_hugepage is set/found, then we don't
need to find anything else? Or can there be more than one
master_hugepage and it is desired to find the "last"?
Is there perhaps a cause/reason to break the loop rather than continue?
IOW: Move the check inside the previous if and use it as a cause to
break the loop.
I think this is ACK-able with a couple of adjustments, but just wanted
to check what was more reasonable first - especially with this < 0 change.
John
if (virBitmapGetBit(hugepage->nodemask, guestNode,
&thisHugepage) < 0) {
/* Ignore this error. It's not an error after all. Well,