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

[Michal Privoznik]

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 at 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