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,