On 03/07/2017 05:23 PM, John Ferlan wrote:
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@redhat.com> --- 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> ... <devices> - <memory model='dimm'> + <memory model='dimm' access='private'>
Hmmm... this type of change almost makes it seem as though access will be required or written out always now.
<target> <size unit='KiB'>524287</size> <node>0</node> @@ -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 <cell/>: + <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?
I don't think so. 'access' is an attribute of backend, not frontend. Therefore guest has no idea what mode is particular NVDIMM in. Thus it's not part of ABI. Michal