6:32 p.m.
On 11/10/2015 08:34 AM, Peter Krempa wrote:
On Tue, Oct 20, 2015 at 17:31:57 -0400, John Ferlan wrote:
>
>
> On 10/16/2015 08:11 AM, Peter Krempa wrote:
>> Adjust the config code so that it does not enforce that target memory
>> node is specified. To avoid breakage, adjust the qemu memory hotplug
>> config checker to disallow such config for now.
>> ---
>> docs/formatdomain.html.in | 5 +++--
>> docs/schemas/domaincommon.rng | 8 +++++---
>> src/conf/domain_conf.c | 10 +++++++---
>> src/qemu/qemu_domain.c | 7 +++++++
>> 4 files changed, 22 insertions(+), 8 deletions(-)
>>
Let's see... can I recall what I was thinking 3 weeks ago (and today was
a rough one - long story).
>
> It almost feels like patches 9 & 10 should precede this one. Perhaps
> easier with the html.in change. Consider this as a review for 8-10 as
> well as some other random thoughts.
They can't be inverted, since the later patches rely on the fact that
the node is now a signed variable. Undoing that would break the
separation. You'll have to live with it.
OK - perhaps I was thinking more along the lines of the docs and how
things were flowing, but sure I see how 10 comes after 8 (perhaps I had
combined them in my mind)
>
> So if I'm reading things correctly, a PPC64 guest "can" supply NUMA
> guest nodes, but it doesn't have to. Is that a fair assumption? It
Well, I'd say that you can hotplug memory on PPC64 without needing NUMA
in any way.
Maybe I was missing that distinction - I cannot recall. But as you
elaborate in the following having NUMA on PPC64 is supported so one
could attach using nodes or not. That makes the rest of my comments
below null and void.
John
> matters mostly to help describe things. If it can have it, then perhaps
> another test (copy qemuxml2argv-memory-hotplug-ppc64-nonuma.xml to
> qemuxml2argv-memory-hotplug-ppc64.xml and add the numa node defs).
Yes that would work, but it would basically test the same code.
>
> If PPC64 doesn't support NUMA guest nodes, then does it make sense in
> post processing code to set targetNode = -1 if ARCH_IS_PPC64? It
That statement is untrue. PPC64 does support guest NUMA nodes. It works
with the current code. This is a usability improvement.
> wouldn't be the first PPC64 specific code. Along the same lines, would
> be ignoring targetNode in patch 9 if PPC64. Furthermore, if PPC64
> doesn't support NUMA guest nodes, I would have expected a check during
> parse - I didn't look that hard for this case. I was merely thinking
> what happens if PPC64 does define NUMA guest nodes and those can be used
> by the memory device.
Um, what?
>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index c88b032..279424d 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -6300,8 +6300,9 @@ qemu-kvm -net nic,model=? /dev/null
>> added memory as a scaled integer.
>> </p>
>> <p>
>> - The mandatory <code>node</code> subelement configures the
guest NUMA
>> - node to attach the memory to.
>> + The <code>node</code> subelement configures the guest NUMA
node to
>> + attach the memory to. Note: Some hypervisors or specific
>> + configurations may require that <code>node</code> is
specified.
>
> How about:
>
> The optional <code>node</code> subelement provides the guest <a
> href="#elementsCPU">cpu NUMA node or cell id</a> to attach the
memory.
In the first sentence this merely drops 'mandatory'. Rewording can be
saved for later.
>
> <p> Note: The hypervisor may require the <code>node</code> to be
> specified for some architectures.</p>
If you enable NUMA for your guest, you have to specify the NUMA node for
the memory module, so your wording is insufficient.
I'll reword it though, but differently.
>
>
> FWIW: It almost feels like we need an example of what is meant - that is
> "For QEMU/KVM, the PowerPC64 pseries guests do not require that the
> <code>node</code> subelement is defined. If not defined, then the memory
> device will be attached to [???]."
... the only memory region the guest is using. Since it doesn't have
NUMA. It can be ommited if and only if NUMA is not enabled.
BTW there's a bug in the code in this regard. The slot wouldn't be
checked on PPC if NUMA was enabled.
>
> BTW: The additional text only makes sense as of patch 10 and it wasn't
> clear if PPC64 "could" support numa guest nodes.
It obviously does.
>
>
>> </p>
>> </dd>
>> </dl>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index f196177..994face 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -4532,9 +4532,11 @@
>> <element name="size">
>> <ref name="scaledInteger"/>
>> </element>
>> - <element name="node">
>> - <ref name="unsignedInt"/>
>> - </element>
>> + <optional>
>> + <element name="node">
>> + <ref name="unsignedInt"/>
>> + </element>
>> + </optional>
>> </interleave>
>> </element>
>> </define>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 514bd8a..a5ab29c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -12520,11 +12520,15 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>> int ret = -1;
>> xmlNodePtr save = ctxt->node;
>> ctxt->node = node;
>> + int rv;
>>
>> - if (virXPathInt("string(./node)", ctxt, &def->targetNode)
< 0 ||
>> - def->targetNode < 0) {
>> + /* initialize to value which marks that the user didn't speify it */
>
> specify
>
> This feels like a need for something like the Tristate{Bool|State}
> structs... Or way to force optional fields to something specific
Not really. Sacrificing the sign bit for this is sufficient here.
Peter