[libvirt] [PATCH 4/5] qemu: Support setting NUMA distances

[John Ferlan]

On 11/22/2017 04:45 AM, Michal Privoznik wrote:
> On 11/22/2017 01:04 AM, John Ferlan wrote:
>>
>>
>> On 11/14/2017 09:47 AM, Michal Privoznik wrote:
>>> Since we already have such support for libxl all we need is qemu
>>> driver adjustment. And a test case.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c                            | 36 +++++++++++-
>>>  .../qemuxml2argv-numatune-distances.args           | 63 +++++++++++++++++++++
>>>  .../qemuxml2argv-numatune-distances.xml            | 65 ++++++++++++++++++++++
>>>  tests/qemuxml2argvtest.c                           |  2 +
>>>  4 files changed, 165 insertions(+), 1 deletion(-)
>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index eb72db33b..8b9daaea3 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -7675,7 +7675,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>>>                      virCommandPtr cmd,
>>>                      qemuDomainObjPrivatePtr priv)
>>>  {
>>> -    size_t i;
>>> +    size_t i, j;
>>>      virQEMUCapsPtr qemuCaps = priv->qemuCaps;
>>>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>>>      char *cpumask = NULL, *tmpmask = NULL, *next = NULL;
>>> @@ -7685,6 +7685,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>>>      int ret = -1;
>>>      size_t ncells = virDomainNumaGetNodeCount(def->numa);
>>>      const long system_page_size = virGetSystemPageSizeKB();
>>> +    bool numa_distances = false;
>>>  
>>>      if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
>>>          !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
>>> @@ -7793,6 +7794,39 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>>>  
>>>          virCommandAddArgBuffer(cmd, &buf);
>>>      }
>>> +
>>> +    /* If NUMA node distance is specified for at least one pair
>>> +     * of nodes, we have to specify all the distances. Even
>>> +     * though they might be the default ones. */
>>> +    for (i = 0; i < ncells; i++) {
>>> +        for (j = 0; j < ncells; j++) {
>>> +            if (!virDomainNumaNodeDistanceSpecified(def->numa, i, j))
>>> +                continue;
>>> +
>>> +            numa_distances = true;
>>> +
>>> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
>>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                               _("setting NUMA distances is not "
>>> +                                 "supported with this qemu"));
>>> +                goto cleanup;
>>> +            }
>>> +        }
>>> +    }
>>
>> Not sure I understand the need for the above double loop.... Even with
>> your adjustment...
>>
>> It would seem that all that's necessary is
>>
>>     for (i < 0; i < ncells; i++) {
>>         if (numa->mem_nodes[i].ndistances > 0)
> 
> It things only were so simple. Thing is, numa is a private struct. We
> need accessors for getting any value. I can't just dereference numa
> struct from qemu code.

Oh right... Could have another accessor too, but your need is a bit more
specific especially since as you point out next qemu will default to 10
and 20...

Still I think with the different name this will make more sense in v2.

> 
> That's why I'm introducing an accessor in 2/5. Moreover, in case all
> values are defaults I wanted to not put them onto the cmd line. The
> advantage would be that it would work even with older qemu which doesn't
> have the capability. I mean, qemu does follow the spec and if no
> distances are provided it uses the ones from the spec (10 and 20).
> 

Nothing is ever so simple, but I did ask if I was in the weeds too
basically because I've learned over time that simplicity and numa are
not synonymous!

John