[libvirt] [PATCH 08/10] conf: Prepare making memory device target node optional

Tue Nov 10 13:34:49 UTC 2015

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151110/5c5c3057/attachment-0001.sig>