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