Re: [libvirt] [PATCH v2 05/14] conf: Introduce @access to <memory/>

On 02/27/2017 08:19 AM, Michal Privoznik wrote: > Now that NVDIMM has found its way into libvirt, users might want > to fine tune some settings for each module separately. One such > setting is 'share=on|off' for the memory-backend-file object. > This setting - just like its name suggest already - enables > sharing the nvdimm module with other applications. Under the hood > it controls whether qemu mmaps() the file as MAP_PRIVATE or > MAP_SHARED. > > Yet again, we have such config knob in domain XML, but it's just > an attribute to numa <cell/>. This does not give fine enough > tuning on per-memdevice basis so we need to have the attribute > for each device too. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 15 +++++- > docs/schemas/domaincommon.rng | 8 ++++ > src/conf/domain_conf.c | 15 +++++- > src/conf/domain_conf.h | 2 + > .../qemuxml2argv-memory-hotplug-nvdimm-access.xml | 56 ++++++++++++++++++++++ > ...qemuxml2xmlout-memory-hotplug-nvdimm-access.xml | 1 + > tests/qemuxml2xmltest.c | 1 + > 7 files changed, 95 insertions(+), 3 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-access.xml > create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm-access.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index b76905cdc..00c0df2ce 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1406,7 +1406,7 @@ > <span class='since'>Since 1.2.9</span> the optional attribute > <code>memAccess</code> can control whether the memory is to be > mapped as "shared" or "private". This is valid only for > - hugepages-backed memory. > + hugepages-backed memory and nvdimm modules. > </p> > > <p> > @@ -7015,7 +7015,7 @@ qemu-kvm -net nic,model=? /dev/null > <pre> > ... > &lt;devices&gt; > - &lt;memory model='dimm'&gt; > + &lt;memory model='dimm' access='private'&gt; Hmmm... this type of change almost makes it seem as though access will be required or written out always now. > &lt;target&gt; > &lt;size unit='KiB'&gt;524287&lt;/size&gt; > &lt;node&gt;0&lt;/node&gt; > @@ -7054,6 +7054,17 @@ qemu-kvm -net nic,model=? /dev/null > </p> > </dd> > > + <dt><code>access</code></dt> > + <dd> > + <p> > + Then there is optional attribute <code>access</code> s/Then there is optional/An optional > + (<span class="since">Since 3.1.0</span>) that allows It'll be "since 3.2.0" at the earliest (don't capitalize Since in the middle of a sentence) > + uses to fine tune mapping of the memory on per module s/allows uses to/provides the capability to/ > + basis. Values are the same as for numa &lt;cell/&gt;: > + <code>shared</code> and <code>private</code>. It's perhaps a nit, but I guess I just read the docs literally rather than interpretively... The "access mode='shared|private'" shows up in the "Memory Backing" section even though there's a NUMA Node Tuning section later. So, I would change "for numa <cell>" to include "Memory Backing" and a link to that, e.g. <a href="#elementsMemoryBacking">Memory Backing</a> within the text > + </p> > + </dd> > + > <dt><code>source</code></dt> > <dd> > <p> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index fafd3e982..78195aae9 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4740,6 +4740,14 @@ > <value>nvdimm</value> > </choice> > </attribute> > + <optional> > + <attribute name="access"> > + <choice> > + <value>shared</value> > + <value>private</value> > + </choice> > + </attribute> > + </optional> > <interleave> > <optional> > <ref name="memorydev-source"/> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 4ffca7dc8..a31114cc7 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -13860,6 +13860,15 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, > } > VIR_FREE(tmp); > > + tmp = virXMLPropString(memdevNode, "access"); > + if (tmp && > + (def->access = virDomainMemoryAccessTypeFromString(tmp)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("invalid access mode '%s'"), tmp); > + goto error; > + } > + VIR_FREE(tmp); > + > /* source */ > if ((node = virXPathNode("./source", ctxt)) && > virDomainMemorySourceDefParseXML(node, ctxt, def) < 0) > @@ -22666,7 +22675,11 @@ virDomainMemoryDefFormat(virBufferPtr buf, > { > const char *model = virDomainMemoryModelTypeToString(def->model); > > - virBufferAsprintf(buf, "<memory model='%s'>\n", model); > + virBufferAsprintf(buf, "<memory model='%s'", model); > + if (def->access) > + virBufferAsprintf(buf, " access='%s'", > + virDomainMemoryAccessTypeToString(def->access)); > + virBufferAddLit(buf, ">\n"); > virBufferAdjustIndent(buf, 2); > > if (virDomainMemorySourceDefFormat(buf, def) < 0) Me thinks this too would need some amount of ABI checks wouldn't it? That is virDomainMemoryDefCheckABIStability for each mem?