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

Re: [libvirt] [PATCHv7 12/18] conf: Introduce cache monitor element in cachetune





On 2018年11月06日 01:26, John Ferlan wrote:

On 10/22/18 4:01 AM, Wang Huaqiang 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 @@
      &lt;cachetune vcpus='0-3'&gt;
        &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt;
        &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt;
+      &lt;monitor level='3' vcpus='1'/&gt;
+      &lt;monitor level='3' vcpus='0-3'/&gt;
+    &lt;/cachetune&gt;
+    &lt;cachetune vcpus='4-5'&gt;
+      &lt;monitor level='3' vcpus='4'/&gt;
+      &lt;monitor level='3' vcpus='5'/&gt;
      &lt;/cachetune&gt;
      &lt;memorytune vcpus='0-3'&gt;
        &lt;node id='0' bandwidth='60'/&gt;
@@ -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
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


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