4:07 a.m.
On Wed, Jan 03, 2018 at 06:50:22PM -0500, John Ferlan wrote:
On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> More info in the documentation, this is basically the XML parsing/formatting
> support, schemas, tests and documentation for the new cputune/cachetune element
> that will get used by following patches.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> docs/formatdomain.html.in | 54 +++++
> docs/schemas/domaincommon.rng | 32 +++
> src/conf/domain_conf.c | 251 +++++++++++++++++++++
> src/conf/domain_conf.h | 13 ++
> tests/genericxml2xmlindata/cachetune-cdp.xml | 36 +++
> .../cachetune-colliding-allocs.xml | 30 +++
> .../cachetune-colliding-tunes.xml | 32 +++
> .../cachetune-colliding-types.xml | 30 +++
> tests/genericxml2xmlindata/cachetune-small.xml | 29 +++
> tests/genericxml2xmlindata/cachetune.xml | 33 +++
> tests/genericxml2xmltest.c | 10 +
> 11 files changed, 550 insertions(+)
> create mode 100644 tests/genericxml2xmlindata/cachetune-cdp.xml
> create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
> create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
> create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-types.xml
> create mode 100644 tests/genericxml2xmlindata/cachetune-small.xml
> create mode 100644 tests/genericxml2xmlindata/cachetune.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 01db83e60820..623860cc0e95 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -689,6 +689,10 @@
> <iothread_quota>-1</iothread_quota>
> <vcpusched vcpus='0-4,^3' scheduler='fifo'
priority='1'/>
> <iothreadsched iothreads='2' scheduler='batch'/>
> + <cachetune vcpus='0-3'>
> + <cache id='0' level='3' type='both'
size='3' unit='MiB'/>
> + <cache id='1' level='3' type='both'
size='3' unit='MiB'/>
> + </cachetune>
> </cputune>
> ...
> </domain>
> @@ -834,6 +838,56 @@
> <span class="since">Since 1.2.13</span>
> </dd>
>
> + <dt><code>cachetune</code><span
class="since">Since 3.10.0</span></dt>
At least 4.0.0 if not 4.1.0
> + <dd>
> + Optional <code>cachetune</code> element can control allocations
for CPU
> + caches using the resctrlfs on the host. Whether or not is this supported
s/resctrlfs/Resource Control File System/
If you search for Resource Control File System you will find lot of unrelevant
stuff. For resctrlfs (or rather resctrl), however, you will get the precise
info you are looking for. It's also called resctrl in the kernel. You mount it
by doing `mount -t resctrl resctrl [-o cdp] /sys/fs/resctrl`, so I think it's
clearer this way (or without the fs at the end).
Although this defaults to /sys/fs/resctrl, I'm not sure if we want
to
state the default or not. It *is* the path we will use though and I
don't believe we have a way to alter that default (yet).
We don't. We don't mention any other sysfs path in the docs. Why would we?
It's defined elsewhere, not in libvirt. And since it's the only place to be
mounted for now (according to the docs) we can have it hardcoded. Just like the
other paths.
> + can be gathered from capabilities where some limitations like minimum
> + size and required granularity are reported as well. The required
> + attribute <code>vcpus</code> specifies to which vCPUs this
allocation
> + applies. A vCPU can only be member of one <code>cachetune</code>
element
> + allocations. Supported subelements are:
> + <dl>
> + <dt><code>cache</code></dt>
> + <dd>
> + This element controls the allocation of CPU cache and has the
> + following attributes:
> + <dl>
> + <dt><code>level</code></dt>
> + <dd>
> + Host cache level from which to allocate.
> + </dd>
> + <dt><code>id</code></dt>
> + <dd>
> + Host cache id from which to allocate.
Oh - should have read this first ;-)... Maybe rename @id internally to
hostCacheID... Shall I assume someone setting this up would know how to
determine how to get these values from the host?
I don't see the point in that. Especially since it's called `cache` in the code.
> + </dd>
> + <dt><code>type</code></dt>
> + <dd>
> + Type of allocation. Can be <code>code</code> for code
> + (instructions), <code>data</code> for data or
<code>both</code>
> + for both code and data (unified). Currently the allocation can
> + be done only with the same type as the host supports, meaning
> + you cannot request <code>both</code> for host with CDP
> + (code/data prioritization) enabled.
> + </dd>
> + <dt><code>size</code></dt>
> + <dd>
> + The size of the region to allocate. The value by default is in
> + bytes, but the <code>unit</code> attribute can be used
to scale
> + the value.
> + </dd>
> + <dt><code>unit</code> (optional)</dt>
> + <dd>
> + If specified it is the unit such as KiB, MiB, GiB, or TiB
> + (described in the <code>memory</code> element
> + for <a href="#elementsMemoryAllocation">Memory
Allocation</a>)
> + in which <code>size</code> is specified, defaults to
bytes.
> + </dd>
> + </dl>
> + </dd>
> + </dl>
> +
> + </dd>
> </dl>
>
>
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bdbee..ec8d760c7971 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2883,6 +2883,17 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
> VIR_FREE(loader);
> }
>
> +static void
> +virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
> +{
> + if (!cachetune)
> + return;
> +
> + virObjectUnref(cachetune->alloc);
> + virBitmapFree(cachetune->vcpus);
> + VIR_FREE(cachetune);
> +}
> +
Two empty lines before and after
This will look awkward when other functions around have one only, but I've
already given up this fight.
> void virDomainDefFree(virDomainDefPtr def)
> {
> size_t i;
[...]
>
> +struct virCachetuneHelperData {
> + virBufferPtr buf;
> + size_t vcpu_id;
> +};
> +
> +static int
> +virDomainCachetuneDefFormatHelper(unsigned int level,
> + virCacheType type,
> + unsigned int cache,
> + unsigned long long size,
> + void *opaque)
> +{
> + const char *unit;
> + virBufferPtr buf = opaque;
> + unsigned long long short_size = virFormatIntPretty(size, &unit);
> +
> + virBufferAsprintf(buf,
> + "<cache id='%u' level='%u'
type='%s' "
> + "size='%llu' unit='%s'/>\n",
> + cache, level, virCacheTypeToString(type),
> + short_size, unit);
A nit, but if the default is "B" and we don't require for parse, then do
we want to Format the output? IOW: why print unit='B'
1) Because that way we can default to something else in the future. We
want to specify everything possible, otherwise we'll get stuck with
something we can't change.
2) All other code does that, it's consistent.
3) It's more readable for users even without reading the docs.
4) It would be more code with no useful output.
> +
> + return 0;
> +}
> +
> +
[...]
> diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml
b/tests/genericxml2xmlindata/cachetune-cdp.xml
> new file mode 100644
> index 000000000000..1331aad06e54
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml
> @@ -0,0 +1,36 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>2</vcpu>
> + <cputune>
> + <cachetune vcpus='0-1'>
> + <cache id='0' level='3' type='code'
size='7680' unit='KiB'/>
> + <cache id='1' level='3' type='data'
size='3840' unit='KiB'/>
> + </cachetune>
> + <cachetune vcpus='2'>
> + <cache id='1' level='3' type='code'
size='6' unit='MiB'/>
> + </cachetune>
> + <cachetune vcpus='3'>
> + <cache id='1' level='3' type='data'
size='6912' unit='KiB'/>
> + </cachetune>
But there's only 2 vcpu's on this guest?
Sure, this will be shrunk and forgotten.
> + </cputune>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <controller type='usb' index='0'/>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0'
model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>
[...]
> diff --git a/tests/genericxml2xmlindata/cachetune.xml
b/tests/genericxml2xmlindata/cachetune.xml
> new file mode 100644
> index 000000000000..0051410aec32
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune.xml
> @@ -0,0 +1,33 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>2</vcpu>
> + <cputune>
> + <cachetune vcpus='0-1'>
> + <cache id='0' level='3' type='both'
size='3' unit='MiB'/>
> + <cache id='1' level='3' type='both'
size='3' unit='MiB'/>
> + </cachetune>
> + <cachetune vcpus='3'>
> + <cache id='0' level='3' type='both'
size='3' unit='MiB'/>
> + </cachetune>
Another one with vcpusid > nvcpus defined above....
I'll give you my:
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Since adjustments are simple going forward. I'm not sure how you should
handle the vcpusid value > nvcpus if that's an issue or not...
I have to send another version anyway. Not only because
virCachetuneHelperData is not used at all, but other things need
adjustments.
John
> + </cputune>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <controller type='usb' index='0'/>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0'
model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>