9:44 a.m.
On 01/23/2018 05:07 AM, Martin Kletzander wrote:
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).
OK that's fine not that big a deal.
> 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.
Tough to recall - review done a month ago, but I have a recollection of
confusion while reading the code. Of course you've been working with it
longer so it's second nature. At this point I was perhaps reflecting a
bit too much.
>> + </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.
I cannot disagree and understand the desire to follow existing style,
but IIRC there's been another agreement at one point in time to go with
the double blank lines between functions. I'm so used to mentioning it
now that it's just habit. I'm sure I don't always follow it 100% in my
patches either. I mention it - if it's done it's done, if it's not,
well I noted it. It's not like there's a syntax-check rule or some
hacking.html requirement.
>> 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.
AOK... I guess it was more of a "reaction" to seeing 'B' (or
'bytes')
rather than 'KiB' or 'MiB' - as in "yeah so".
John
>> +
>> + 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>