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