On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> Introduce resource monitoring group in domain configuration file
> to support CPU cache monitoring technology (CMT).
>
> Domain rng file changes, supporting following types of resource
> monitoring group regarding the allocation regin it belongs to:
> 1. monitoring group that working for partial working thread of
> current allocation:
> e.g. "<monitor vcpus='0'/>" creates monitoring group special
> for vcpu '0' while an allocation group is created for vcpus
> of '0' *and* '1'.
> 2. monitoring group for whole vcpu set of current allocation:
> e.g. "<monitor vcpus='0-1'/>" creates monitoring group for
> all vcpus belonging to current allocation.
> 3. monitoring group for vcpu(s) that does not have dedicated
> allocation group:
> e.g. "<monitor vcpus='3'/>" creates a monitoring group but
> no resource control applied to it.
>
> <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'/>
> + <monitor vcpus='0-1'/>
> + <monitor vcpus='0'/>
> </cachetune>
> <cachetune vcpus='2'>
> <cache id='1' level='3' type='code' size='6' unit='MiB'/>
> + <monitor vcpus='2'/>
> </cachetune>
> <cachetune vcpus='3'>
> + <monitor vcpus='3'/>
> </cachetune>
> </cputune>
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> ---
> docs/formatdomain.html.in | 14 ++-
> docs/schemas/domaincommon.rng | 11 +-
> src/conf/domain_conf.c | 131 ++++++++++++++++++---
> src/conf/domain_conf.h | 20 ++++
> tests/genericxml2xmlindata/cachetune-cdp.xml | 2 +
> .../cachetune-colliding-monitors.xml | 36 ++++++
> tests/genericxml2xmlindata/cachetune-small.xml | 1 +
> tests/genericxml2xmlindata/cachetune.xml | 3 +
> tests/genericxml2xmltest.c | 4 +
> 9 files changed, 204 insertions(+), 18 deletions(-)
> create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitors.xml
>
Getting more difficult to keep these changes and my suggested
alterations in the same context.
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0cbf570..33d2890 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -758,6 +758,7 @@
> <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'/>
> + <monitor vcpus='0-1'/>
Interesting that for domain <monitor> is at the same level (or child
relationship) as <cache>, but for the capabilities it was a child of the
<bank> which honestly is confusing.
> </cachetune>
> <memorytune vcpus='0-3'>
> <node id='0' bandwidth='60'/>
> @@ -942,8 +943,8 @@
> <dl>
> <dt><code>cache</code></dt>
> <dd>
> - This element controls the allocation of CPU cache and has the
> - following attributes:
> + This optional element controls the allocation of CPU cache and has
> + the following attributes:
So <cache> is optional now?! That needs to be separate.
> <dl>
> <dt><code>level</code></dt>
> <dd>
> @@ -977,6 +978,15 @@
> </dd>
> </dl>
> </dd>
> + <dt><code>monitor</code></dt>
> + <dd>
> + The optional element <code>monitor</code> creates the cahce
cache
> + monitoring group(s) for current cache allocation group. The required
> + attribute <code>vcpus</code> specifies to which vCPUs this
> + monitoring group applies. A vCPU can only be member of one
> + <code>cachetune</code> element allocation. And no overlap is
> + permitted.
And it only works for L3 <cache>'s right?
> + </dd>
> </dl>
> </dd>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f176538..83fb9b7 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -956,7 +956,7 @@
> <attribute name="vcpus">
> <ref name='cpuset'/>
> </attribute>
> - <oneOrMore>
> + <zeroOrMore>
!! Needs to be separate
> <element name="cache">
> <attribute name="id">
> <ref name='unsignedInt'/>
> @@ -980,7 +980,14 @@
> </attribute>
> </optional>
> </element>
> - </oneOrMore>
> + </zeroOrMore>
> + <zeroOrMore>
> + <element name="monitor">
> + <attribute name="vcpus">
> + <ref name='cpuset'/>
> + </attribute>
> + </element>
> + </zeroOrMore>
> </element>
> </zeroOrMore>
> <zeroOrMore>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9a65655..304a94e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2969,13 +2969,30 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>
>
> static void
> +virDomainResctrlMonFree(virDomainResctrlMonitorPtr monitor)
> +{
> + if (!monitor)
> + return;
> +
> + VIR_FREE(monitor->id);
> + virBitmapFree(monitor->vcpus);
> + VIR_FREE(monitor);
> +}
> +
> +
> +static void
> virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
> {
> + size_t i = 0;
> +
> if (!resctrl)
> return;
>
> virObjectUnref(resctrl->alloc);
> virBitmapFree(resctrl->vcpus);
> + for (i = 0; i < resctrl->nmonitors; i++)
> + virDomainResctrlMonFree(resctrl->monitors[i]);
> + VIR_FREE(resctrl->monitors);
> VIR_FREE(resctrl);
> }
>
> @@ -19298,6 +19315,71 @@ virDomainResctrlAppend(virDomainDefPtr def,
>
>
> static int
> +virDomainResctrlParseMonitor(virDomainDefPtr def,
> + xmlXPathContextPtr ctxt,
> + xmlNodePtr node,
> + virDomainResctrlDefPtr resctrl)
> +{
> + xmlNodePtr oldnode = ctxt->node;
> + virBitmapPtr vcpus = NULL;
> + char *id = NULL;
> + int vcpu = -1;
> + char *vcpus_str = NULL;
> + virDomainResctrlMonitorPtr tmp_domresmon = NULL;
The "tmp_" prefix doesn't seem necessary...
> + int ret = -1;
> +
> + if (!resctrl || !resctrl->vcpus || !resctrl->alloc)
> + return -1;
> +
> + ctxt->node = node;
> +
> + if (VIR_ALLOC(tmp_domresmon) < 0)
> + goto cleanup;
We don't need/use this until ... [1]
> +
> + if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0)
> + goto cleanup;
> +
> + /* empty monitoring group is not allowed */
> + if (virBitmapIsAllClear(vcpus))
So we'll fail without an error? How is the consumer supposed to know
that providing the empty set isn't valid?
> + goto cleanup;
> +
> + while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) {
> + if (!virBitmapIsBitSet(resctrl->vcpus, vcpu))
Again fail without an error? How would someone know that what they've
provided doesn't 'work' properly because the resctrl->vcpus doesn't have
that vcpu in it's list?
> + goto cleanup;
> + }
> +
> + vcpus_str = virBitmapFormat(vcpus);
> + if (!vcpus_str)
> + goto cleanup;
> +
[1] right about here
> + if (virAsprintf(&id, "vcpus_%s", vcpus_str) < 0)
> + goto cleanup;
> +
> + if (VIR_STRDUP(tmp_domresmon->id, id) < 0)
> + goto cleanup;
The two steps are unnecessary since @id is VIR_FREE'd anyway. Let's just:
if (virAsprintf(&domresmon->id, "vcpus_%s", vcpus_str) < 0)
goto cleanup;
> +
> + tmp_domresmon->vcpus = vcpus;
> +
> + if (VIR_APPEND_ELEMENT(resctrl->monitors,
> + resctrl->nmonitors,
> + tmp_domresmon) < 0)
> + goto cleanup;
> +
> + if (virResctrlAllocSetMonitor(resctrl->alloc, id) < 0)
> + goto cleanup;
> +
> + tmp_domresmon = NULL;
Shouldn't this go after VIR_APPEND_ELEMENT? otherwise we could end up in
cleanup with it on resctrl->monitors *and* virDomainResctrlMonFree is
called.
> + ret = 0;
> + cleanup:
> + ctxt->node = oldnode;
> + VIR_FREE(id);
> + VIR_FREE(vcpus_str);
> + virDomainResctrlMonFree(tmp_domresmon);
> + return ret;
> +}
> +
> +
> +static int
> virDomainCachetuneDefParse(virDomainDefPtr def,
> xmlXPathContextPtr ctxt,
> xmlNodePtr node,
> @@ -19313,6 +19395,9 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> int n;
> int ret = -1;
>
> + if (VIR_ALLOC(tmp_resctrl) < 0)
> + return -1;
> +
> ctxt->node = node;
>
> if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0)
> @@ -19347,30 +19432,40 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> goto cleanup;
> }
>
> - if (virResctrlAllocIsEmpty(alloc)) {
> - ret = 0;
> + tmp_resctrl->vcpus = vcpus;
> + tmp_resctrl->alloc = virObjectRef(alloc);
> +
> + VIR_FREE(nodes);
> + ctxt->node = node;
> +
> + if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot extract monitor nodes under cachetune"));
> goto cleanup;
> }
>
> - if (VIR_ALLOC(tmp_resctrl) < 0)
> - goto cleanup;
> + for (i = 0; i < n; i++) {
> + if (virDomainResctrlParseMonitor(def, ctxt,
> + nodes[i], tmp_resctrl) < 0)
Hmmm - something slightly different with this ordering which makes my
previous patch comments not work as well.
>
> - tmp_resctrl->vcpus = vcpus;
> - tmp_resctrl->alloc = virObjectRef(alloc);
> + goto cleanup;
> + }
> +
> + if (virResctrlAllocIsEmpty(alloc)) {
> + VIR_WARN("cachetune: resctrl alloc is empty");
> + ret = 0;
> + goto cleanup;
> + }
So if I reconsider slightly my previous patch because now we need a trip
through virDomainResctrlParseMonitor, we could have:
virDomainResctrlDefNew(alloc, vcpus):
if (VIR_ALLOC(resctrl) < 0)
return NULL;
resctrl->alloc = virObjectRef(alloc);
resctrl->vcpus = vcpus;
return resctrl;
Back in the caller we have:
if (!(resctrl = virDomainResctrlDefNew(alloc, vcpus)))
goto cleanup;
alloc = NULL;
vcpus = NULL;
Then calling virDomainResctrlAppend using @resctrl:
if (virDomainResctrlAppend(def, node, resctrl, flags) < 0)
goto cleanup;
resctrl = NULL;
...
cleanup:
...
virDomainResctrlDefFree(resctrl);
I think doing this gives the flexibility to this code to make that
virDomainResctrlParseMonitor call before appending the new resctrl
There's so much changing now - I'm just going to stop here and see how
things shake out in the next series.
One other note first though - in patch 10 in
qemuDomainGetStatsCpuResource the "unsigned int nmonitor = NULL;" failed
the compiler rather spectacularly...
John
>
> if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
> goto cleanup;
>
> - alloc = NULL;
> - vcpus = NULL;
> tmp_resctrl = NULL;
>
> ret = 0;
> cleanup:
> ctxt->node = oldnode;
> - virObjectUnref(alloc);
> - VIR_FREE(tmp_resctrl);
> - virBitmapFree(vcpus);
> + virDomainResctrlDefFree(tmp_resctrl);
> VIR_FREE(nodes);
> return ret;
> }
> @@ -19588,10 +19683,8 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
> ret = 0;
> cleanup:
> ctxt->node = oldnode;
> - virBitmapFree(vcpus);
> - virObjectUnref(alloc);
> - VIR_FREE(tmp_resctrl);
> VIR_FREE(nodes);
> + virDomainResctrlDefFree(tmp_resctrl);
> return ret;
> }
>
> @@ -27394,6 +27487,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
> {
> virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> char *vcpus = NULL;
> + size_t i = 0;
> int ret = -1;
>
> virBufferSetChildIndent(&childrenBuf, buf);
> @@ -27405,6 +27499,15 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
> if (virBufferCheckError(&childrenBuf) < 0)
> goto cleanup;
>
> + for (i = 0; i < resctrl->nmonitors; i++) {
> + vcpus = virBitmapFormat(resctrl->monitors[i]->vcpus);
> + if (!vcpus)
> + goto cleanup;
> +
> + virBufferAsprintf(&childrenBuf, "<monitor vcpus='%s'/>\n", vcpus);
> + VIR_FREE(vcpus);
> + }
> +
> if (!virBufferUse(&childrenBuf)) {
> ret = 0;
> goto cleanup;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index c0ad072..797b4bd 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2235,12 +2235,31 @@ struct _virDomainCputune {
> };
>
>
> +typedef enum {
> + VIR_DOMAIN_RESCTRL_MONITOR_CACHE,
> + VIR_DOMAIN_RESCTRL_MONITOR_MEMBW,
> + VIR_DOMAIN_RESCTRL_MONITOR_CACHE_MEMBW,
> +
> + VIR_DOMAIN_RESCTRL_MONITOR_LAST
> +} virDomainResctrlMonType;
> +
> +typedef struct _virDomainResctrlMonitor virDomainResctrlMonitor;
> +typedef virDomainResctrlMonitor *virDomainResctrlMonitorPtr;
> +struct _virDomainResctrlMonitor {
> + int type; /* virDomainResctrlMonType*/
> + char *id;
> + virBitmapPtr vcpus;
> +};
> +
> +
> typedef struct _virDomainResctrlDef virDomainResctrlDef;
> typedef virDomainResctrlDef *virDomainResctrlDefPtr;
>
> struct _virDomainResctrlDef {
> virBitmapPtr vcpus;
> virResctrlAllocPtr alloc;
> + virDomainResctrlMonitorPtr *monitors;
> + size_t nmonitors;
> };
>
>
> @@ -3455,6 +3474,7 @@ VIR_ENUM_DECL(virDomainIOMMUModel)
> VIR_ENUM_DECL(virDomainVsockModel)
> VIR_ENUM_DECL(virDomainShmemModel)
> VIR_ENUM_DECL(virDomainLaunchSecurity)
> +VIR_ENUM_DECL(virDomainResctrlMonType)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml b/tests/genericxml2xmlindata/cachetune-cdp.xml
> index 9718f06..b257fd5 100644
> --- a/tests/genericxml2xmlindata/cachetune-cdp.xml
> +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml
> @@ -8,9 +8,11 @@
> <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'/>
> + <monitor vcpus='0-1'/>
> </cachetune>
> <cachetune vcpus='2'>
> <cache id='1' level='3' type='code' size='6' unit='MiB'/>
> + <monitor vcpus='2'/>
> </cachetune>
> <cachetune vcpus='3'>
> <cache id='1' level='3' type='data' size='6912' unit='KiB'/>
> diff --git a/tests/genericxml2xmlindata/cachetune-colliding-monitors.xml b/tests/genericxml2xmlindata/cachetune-colliding-monitors.xml
> new file mode 100644
> index 0000000..7526070
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune-colliding-monitors.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'>4</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'/>
> + <monitor vcpus='0-2'/>
> + <monitor vcpus='0'/>
> + </cachetune>
> + <cachetune vcpus='3'>
> + <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> + <monitor vcpus='3'/>
> + </cachetune>
> + </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-small.xml b/tests/genericxml2xmlindata/cachetune-small.xml
> index ab2d9cf..aa7b2c3 100644
> --- a/tests/genericxml2xmlindata/cachetune-small.xml
> +++ b/tests/genericxml2xmlindata/cachetune-small.xml
> @@ -7,6 +7,7 @@
> <cputune>
> <cachetune vcpus='0-1'>
> <cache id='0' level='3' type='both' size='768' unit='KiB'/>
> + <monitor vcpus='0-1'/>
> </cachetune>
> </cputune>
> <os>
> diff --git a/tests/genericxml2xmlindata/cachetune.xml b/tests/genericxml2xmlindata/cachetune.xml
> index 645cab7..52e95bc 100644
> --- a/tests/genericxml2xmlindata/cachetune.xml
> +++ b/tests/genericxml2xmlindata/cachetune.xml
> @@ -8,9 +8,12 @@
> <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'/>
> + <monitor vcpus='0-1'/>
> + <monitor vcpus='0'/>
> </cachetune>
> <cachetune vcpus='3'>
> <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> + <monitor vcpus='3'/>
> </cachetune>
> </cputune>
> <os>
> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index e6d4ef2..bc2fc50 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -140,11 +140,15 @@ mymain(void)
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> DO_TEST_FULL("cachetune-colliding-types", false, true,
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> + DO_TEST_FULL("cachetune-colliding-monitors", false, true,
> + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> DO_TEST("memorytune");
> DO_TEST_FULL("memorytune-colliding-allocs", false, true,
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> DO_TEST_FULL("memorytune-colliding-cachetune", false, true,
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> + DO_TEST_FULL("cachetune-colliding-monitors", false, true,
> + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
>
> DO_TEST("tseg");
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
