Re: [PATCH v2 07/14] qemu: Introduce label-size for NVDIMMs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/07/2017 05:55 PM, John Ferlan wrote:
> 
> 
> On 02/27/2017 08:19 AM, Michal Privoznik wrote:
>> For NVDIMM devices it is optionally possible to specify the size
>> of internal storage for namespaces. Namespaces are a feature that
>> allows users to partition the NVDIMM for different uses.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>>  docs/formatdomain.html.in                          |  9 ++++
>>  docs/schemas/domaincommon.rng                      |  7 +++
>>  src/conf/domain_conf.c                             | 19 +++++++
>>  src/conf/domain_conf.h                             |  1 +
>>  src/qemu/qemu_command.c                            |  3 ++
>>  .../qemuxml2argv-memory-hotplug-nvdimm-label.args  | 26 ++++++++++
>>  .../qemuxml2argv-memory-hotplug-nvdimm-label.xml   | 59 ++++++++++++++++++++++
>>  tests/qemuxml2argvtest.c                           |  2 +
>>  .../qemuxml2xmlout-memory-hotplug-nvdimm-label.xml |  1 +
>>  tests/qemuxml2xmltest.c                            |  1 +
>>  10 files changed, 128 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-label.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-label.xml
>>  create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm-label.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 00c0df2ce..4a078b577 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -7038,6 +7038,9 @@ qemu-kvm -net nic,model=? /dev/null
>>      &lt;target&gt;
>>        &lt;size unit='KiB'&gt;524288&lt;/size&gt;
>>        &lt;node&gt;1&lt;/node&gt;
>> +      &lt;label&gt;
>> +        &lt;size unit='KiB'&gt;128&lt;/size&gt;
>> +      &lt;/label&gt;
>>      &lt;/target&gt;
>>    &lt;/memory&gt;
>>  &lt;/devices&gt;
>> @@ -7117,6 +7120,12 @@ qemu-kvm -net nic,model=? /dev/null
>>            attach the memory to. The element shall be used only if the guest has
>>            NUMA nodes configured.
>>          </p>
>> +        <p>
>> +          For NVDIMM type devices one can optionally use
>> +          <code>label</code> and its subelement <code>size</code>
>> +          to configure the size of namespaces label storage
>> +          within the NVDIMM module.
> 
> and the "unit=" is also required, right?

No. By default the unit is KiB. If users want to use different one, they
have to specify it in @unit attribute. This is just like domain memory.
That's why we can use the same parsing function.

> 
>> +        </p>
>>        </dd>
>>      </dl>
>>  
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 78195aae9..aafc731f5 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -4800,6 +4800,13 @@
>>              <ref name="unsignedInt"/>
>>            </element>
>>          </optional>
>> +        <optional>
>> +          <element name="label">
>> +            <element name="size">
>> +              <ref name="scaledInteger"/>
>> +            </element>
>> +          </element>
>> +        </optional>
>>        </interleave>
>>      </element>
>>    </define>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index a31114cc7..7840f8e02 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13824,6 +13824,18 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>>                               &def->size, true, false) < 0)
>>          goto cleanup;
>>  
>> +    if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>> +        if (virDomainParseMemory("./label/size", "./label/size/@unit", ctxt,
>> +                                 &def->labelsize, false, false) < 0)
> 
> So one could provide a fairly large size for this?

Yeah, if they have enough (disk/actual nvdimm) space. Now that I'm
thinking about this, maybe we can at least check if the label-size is
not greater than the size of nvdimm itself.

> 
> Why is '@required' false?

Because it is not required to specify label size.

> 
>> +            goto cleanup;
>> +
>> +        if (def->labelsize && def->labelsize < 128) {
> 
> This makes it seem labelsize is optional, but it's not clear (to me at
> least) from the description above whether must provide the size as well
> as the label.
> 
> Of course as I read on - labelsize is expected.... Let's face it, if
> label is provided and labelsize is 0, then well not much is going to be
> allowed is it?

I think you can still use it as a data storage inside the guest. Labels
were not introduced in qemu at the same time as NVDIMM, so clearly you
can use NVDIMMs even without labels.

> 
> 
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("nvdimm label must be at least 128KiB"));
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      ret = 0;
>>  
>>   cleanup:
>> @@ -22663,6 +22675,13 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
>>      virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->size);
>>      if (def->targetNode >= 0)
>>          virBufferAsprintf(buf, "<node>%d</node>\n", def->targetNode);
>> +    if (def->labelsize) {
>> +        virBufferAddLit(buf, "<label>\n");
>> +        virBufferAdjustIndent(buf, 2);
>> +        virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->labelsize);
> 
> There's no check here if labelsize wasn't provided.

Yes, there is.

> 
>> +        virBufferAdjustIndent(buf, -2);
>> +        virBufferAddLit(buf, "</label>\n");
>> +    }
>>  
>>      virBufferAdjustIndent(buf, -2);
>>      virBufferAddLit(buf, "</target>\n");
> 
> Similar comments from before about ABI...

This might actually require ABI check as the label-size is visible
inside the guest. Will update it.

> 
> Additionally if NVDIMM's are included in the total memory allocation's
> (from my comments in patch2), wouldn't the size of this label need to
> also be included?
> 

No. This is not some additional memory. Label size specifies the size of
a portion of NVDIMM that is used to store labels. For instance, if I
have 512MiB NVDIMM and set label-size to 2MiB, there's only 510MiB
available for data. If the label-size is 128MiB then there's only 128MiB
left for data.

>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 87516ca69..0e68f5770 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2013,6 +2013,7 @@ struct _virDomainMemoryDef {
>>      int model; /* virDomainMemoryModel */
>>      int targetNode;
>>      unsigned long long size; /* kibibytes */
>> +    unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
>>  
>>      virDomainDeviceInfo info;
>>  };
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 287387055..91ace8e38 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3440,6 +3440,9 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
>>          if (mem->targetNode >= 0)
>>              virBufferAsprintf(&buf, "node=%d,", mem->targetNode);
>>  
>> +        if (mem->labelsize)
>> +            virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
>> +
> 
> And this code checks for labelsize using the "assumption" (I suppose
> that if label, then we have a labelsize too.

No. If mem->labelsize then mem->labelsize. Or maybe I'm misunderstanding
something?

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux