Re: [libvirt] [PATCHv4 2/3] conf: Add new xml elements for file memorybacking support

-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Saturday, January 28, 2017 3:03 PM To: Safka, JaroslavX <jaroslavx.safka(a)intel.com>; libvir-list(a)redhat.com Cc: Mooney, Sean K <sean.k.mooney(a)intel.com>; Ptacek, MichalX <michalx.ptacek(a)intel.com&gt; Subject: Re: [libvirt] [PATCHv4 2/3] conf: Add new xml elements for file memorybacking support On 13.12.2016 13:12, Jaroslav Safka wrote: > This first change introduces new xml elements for file based > memorybacking support and their parsing. > It allows vhost-user to be used without hugepages. > > New xml elements: > <memoryBacking> > <source type="file|anonymous"/> > <access mode="shared|private"/> > <allocation mode="immediate|ondemand"/> </memoryBacking> > --- > docs/formatdomain.html.in | 9 ++ > docs/schemas/domaincommon.rng | 30 +++++ > src/conf/domain_conf.c | 133 ++++++++++++++++----- > src/conf/domain_conf.h | 22 ++++ > .../qemuxml2argv-memorybacking-set.xml | 24 ++++ > .../qemuxml2argv-memorybacking-unset.xml | 24 ++++ > .../qemuxml2xmlout-memorybacking-set.xml | 32 +++++ > .../qemuxml2xmlout-memorybacking-unset.xml | 32 +++++ > tests/qemuxml2xmltest.c | 3 + > 9 files changed, 276 insertions(+), 33 deletions(-) create mode > 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml > create mode 100644 > tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml > create mode 100644 > tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 9218eec..fc194bf 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -903,6 +903,9 @@ > &lt;/hugepages&gt; > &lt;nosharepages/&gt; > &lt;locked/&gt; > + &lt;source type="file|anonymous"/&gt; > + &lt;access mode="shared|private"/&gt; > + &lt;allocation mode="immediate|ondemand"/&gt; > &lt;/memoryBacking&gt; > ... > &lt;/domain&gt; > @@ -942,6 +945,12 @@ > most of the host's memory). Doing so may be dangerous to both the > domain and the host itself since the host's kernel may run out of > memory. <span class="since">Since 1.0.6</span></dd> > + <dt><code>source</code></dt> > + <dd>In this attribute you can switch to file memorybacking or keep default anonymous.</dd> > + <dt><code>access</code></dt> > + <dd>Specify if memory is shared or private. This can be overridden per numa node by <code>memAccess</code></dd> > + <dt><code>allocation</code></dt> > + <dd>Specify when allocate the memory</dd> > </dl> > > > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng index c004233..363ee2f 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -560,6 +560,36 @@ > <empty/> > </element> > </optional> > + <optional> > + <element name="source"> > + <attribute name="type"> > + <choice> > + <value>file</value> > + <value>anonymous</value> > + </choice> > + </attribute> > + </element> > + </optional> > + <optional> > + <element name="access"> > + <attribute name="mode"> > + <choice> > + <value>shared</value> > + <value>private</value> > + </choice> > + </attribute> > + </element> > + </optional> > + <optional> > + <element name="allocation"> > + <attribute name="mode"> > + <choice> > + <value>immediate</value> > + <value>ondemand</value> > + </choice> > + </attribute> > + </element> > + </optional> > </interleave> > </element> > </optional> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index > b0bd38d..c54fc97 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -837,6 +837,16 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, > "abort", > "pivot") > > +VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST, > + "none", > + "file", > + "anonymous") > + > +VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, > + "none", > + "immediate", > + "ondemand") > + > VIR_ENUM_IMPL(virDomainLoader, > VIR_DOMAIN_LOADER_TYPE_LAST, > "rom", > @@ -16528,48 +16538,93 @@ virDomainDefParseXML(xmlDocPtr xml, > } > VIR_FREE(tmp); > > - if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("cannot extract hugepages nodes")); > - goto error; > + tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt); > + if (tmp) { > + if ((def->mem.source = > + virDomainMemorySourceTypeFromString(tmp)) < 0) { This will not fly. def->mem.source is typeof enum virDomainMemorySource which can have values 0, 1 or 2. Now, should virDomainMemorySourceTypeFromString() return an error (-1) (because of unknown @type value), this gets squeezed into the enum range, which always falls into 0-2 range. IOW, this condition will never be true. That's why we have all those: int blah; /* enum virMyAwesomeEnum */ lines in the definition. Had the @source been an int, this starts working. Except, we might want to disallow "none" and thus the condition should be "<= 0". This also apply for the other two variables.

> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown memoryBacking/source/type '%s'"), tmp); > + goto error; > + } > + VIR_FREE(tmp); > } ACK with that change. Michal