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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list