7:34 a.m.
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(-)
>
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.
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.
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