On 2018年11月06日 01:26, John Ferlan wrote:
Introducing <monitor> element under <cachetune> to represent a cache monitor.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- docs/formatdomain.html.in | 26 +++ docs/schemas/domaincommon.rng | 10 + src/conf/domain_conf.c | 234 ++++++++++++++++++++- src/conf/domain_conf.h | 11 + tests/genericxml2xmlindata/cachetune-cdp.xml | 3 + .../cachetune-colliding-monitor.xml | 30 +++ tests/genericxml2xmlindata/cachetune-small.xml | 7 + tests/genericxml2xmltest.c | 2 + 8 files changed, 322 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitor.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b1651e3..2fd665c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -759,6 +759,12 @@ <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 level='3' vcpus='1'/> + <monitor level='3' vcpus='0-3'/> + </cachetune> + <cachetune vcpus='4-5'> + <monitor level='3' vcpus='4'/> + <monitor level='3' vcpus='5'/> </cachetune> <memorytune vcpus='0-3'> <node id='0' bandwidth='60'/> @@ -978,6 +984,26 @@ </dd> </dl> </dd> + <dt><code>monitor</code></dt> + <dd> + The optional element <code>monitor</code> creates the cache + monitor(s) for current cache allocation and has the following + required attributes: + <dl> + <dt><code>level</code></dt> + <dd> + Host cache level the monitor belongs to. + </dd> + <dt><code>vcpus</code></dt> + <dd> + vCPU list the monitor applies to. A monitor's vCPU list + can only be the member(s) of the vCPU list of associating
On 10/22/18 4:01 AM, Wang Huaqiang wrote: the associated
Thanks.
+ allocation. The default monitor has the same vCPU list as the + associating allocation. For non-default monitors, there associated
Thanks.
+ are no vCPU overlap permitted. For non-default monitors, overlapping vCPUs are not permitted.
Thanks.
+ </dd> + </dl> + </dd> </dl> </dd>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a068d4d..01f795f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2955,13 +2955,31 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
[...]
@@ -19026,7 +19215,14 @@ virDomainCachetuneDefParse(virDomainDefPtr def, if (!resctrl) goto cleanup;
- if (virResctrlAllocIsEmpty(alloc)) { + if (virDomainResctrlMonDefParse(def, ctxt, node, + VIR_RESCTRL_MONITOR_TYPE_CACHE, + resctrl) < 0) + goto cleanup; + + /* If no <cache> element or <monitor> element in <cachetune>, do not + * append any resctrl element */ + if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) { Swap order since *IsEmpty is more compute intensive, also change to:
if (resctrl->nmonitors == 0 &&
Agree.
ret = 0; goto cleanup; } @@ -27063,12 +27259,41 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
static int +virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon, + virResctrlMonitorType tag, + virBufferPtr buf) +{ + char *vcpus = NULL; + + if (domresmon->tag != tag) + return 0; + + virBufferAddLit(buf, "<monitor "); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + virBufferAsprintf(buf, "level='%u' ", + VIR_DOMAIN_RESCTRL_MONITOR_CACHELEVEL); + }
So is this because when <memtune> is introduced it won't use a cache level, right? Just trying to recall (and perhaps add a comment).
Yes. 'level' is just for cachetune monitor. Planned to add the comment when extending this function to support memtune monitor, mentioning the memtune monitor ( tag == VIR_RESCTRL_MONITOR_TYPE_BW) here will cause confusing.
+ + vcpus = virBitmapFormat(domresmon->vcpus); + if (!vcpus) + return -1; + + virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus); + + VIR_FREE(vcpus); + return 0; +} + + [...]
I can fix the minor nits, just ack them...
Thanks please help fix.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Thanks for fixing and the review, I will make changes in my code. Huaqiang