Re: [PATCHv5 13/19] conf: Add resctrl monitor configuration

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

 




On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Introducing <monitor> element under <cachetune> to represent
> a cache monitor.
> 
> Supports two kind of monitors, which are, monitor under default
> allocation or monitor under particular allocation.

I still don't see what the big difference is with singling out default.
Does it really matter - what advantage is there. Having a monitor for
'n' vcpus is a choice.


> 
> Monitor supervises the cache or memory bandwidth usage for

At this point memory bandwidth is not in play...

> interested vcpu thread set, if the vcpu thread set is belong to some
> resctrl allocation, then the monitor will be created under this
> allocation, that is, creating a resctrl monitoring group directory under
> the directory of '@alloc->path/mon_group'. Otherwise, the monitor
> will be created under default allocation.

This is becoming increasing difficult to describe/decipher - makes me
wonder who would really use it.

> 
> For default allocation monitor, it will have such kind of XML layout:
> 
>     <cachetune vcpus='1'>
>       <monitor level=3 vcpus='1'/>
>     </cachetune>
> 
> For other type monitor, the XML layout will be something like:
> 
>     <cachetune vcpus='2-4'>
>       <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='2'/>
>     </cachetune>
> 

Since all we get is the "cache_occupancy" that would seem to me to be
most important when there is a <cache> bank, right?  So what would the
first (or your default style) collect/display. Or does cache not really
matter.

IOW: What is cache_occupancy measuring? Each cache? The entire thing? If
there's no cache elements, then what?

I honestly think this just needs to be simplified as much as possible.
When you monitor specific vcpus within a cachetune, then you get what?
If the cachetune has no specific cache entries, you get what?  If you
monitor multiple vcpus within a cachetune then you get what? (?An
aggregation of all?).  This whole default and specific description
doesn't make sense.

> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> ---
>  docs/formatdomain.html.in                          |  26 +++
>  docs/schemas/domaincommon.rng                      |  10 +
>  src/conf/domain_conf.c                             | 217 ++++++++++++++++++++-
>  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, 301 insertions(+), 5 deletions(-)
>  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
> +                allocation. The default monitor has the same vCPU list as the
> +                associating allocation. For non-default monitors, there
> +                are no vCPU overlap permitted.
> +              </dd>
> +            </dl>
> +          </dd>
>          </dl>
>        </dd>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 5c533d6..7ce49d3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -981,6 +981,16 @@
>                  </optional>
>                </element>
>              </zeroOrMore>
> +            <zeroOrMore>
> +              <element name="monitor">
> +                <attribute name="level">
> +                  <ref name='unsignedInt'/>
> +                </attribute>
> +                <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 9a514a6..4f4604f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2955,13 +2955,30 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>  
>  
>  static void
> +virDomainResctrlMonDefFree(virDomainResctrlMonDefPtr domresmon)
> +{
> +    if (!domresmon)
> +        return;
> +
> +    virBitmapFree(domresmon->vcpus);
> +    virObjectUnref(domresmon->instance);

    VIR_FREE(domresmon);

> +}
> +
> +
> +static void
>  virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
>  {
> +    size_t i = 0;
> +
>      if (!resctrl)
>          return;
>  
> +    for (i = 0; i < resctrl->nmonitors; i++)
> +        virDomainResctrlMonDefFree(resctrl->monitors[i]);
> +
>      virObjectUnref(resctrl->alloc);
>      virBitmapFree(resctrl->vcpus);
> +    VIR_FREE(resctrl->monitors);
>      VIR_FREE(resctrl);
>  }
>  
> @@ -18919,6 +18936,154 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>      return ret;
>  }
>  

Two blank lines

> +/* Checking if the monitor's vcpus is conflicted with existing allocation
> + * and monitors.
> + *
> + * Returns 1 if @vcpus equals to @resctrl->vcpus, means it is a default
> + * monitor. Returns - 1 if a conflict found. Returns 0 if no conflict and
> + * @vcpus is not equal to @resctrl->vcpus.
> + * */
> +static int
> +virDomainResctrlMonValidateVcpu(virDomainResctrlDefPtr resctrl,
> +                                virBitmapPtr vcpus)

This should be *ValidateVcpus.

> +{
> +    size_t i = 0;
> +    int vcpu = -1;
> +
> +    if (virBitmapIsAllClear(vcpus)) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("vcpus is empty"));
> +        return -1;
> +    }
> +
> +    while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) {
> +        if (!virBitmapIsBitSet(resctrl->vcpus, vcpu)) {
> +            virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                           _("Monitor vcpus conflicts with allocation"));
> +            return -1;
> +        }
> +    }
> +
> +    if (resctrl->alloc && virBitmapEqual(vcpus, resctrl->vcpus))

The ->alloc check is confusing in light of having a monitor as a child
of cachetune.

> +        return 1;
> +
> +    for (i = 0; i < resctrl->nmonitors; i++) {
> +        if (virBitmapEqual(resctrl->vcpus, resctrl->monitors[i]->vcpus))
> +            continue;
> +
> +        if (virBitmapOverlaps(vcpus, resctrl->monitors[i]->vcpus)) {
> +            virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                           _("Monitor vcpus conflicts with monitors"));
> +
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int
> +virDomainResctrlMonDefParse(virDomainDefPtr def,
> +                            xmlXPathContextPtr ctxt,
> +                            xmlNodePtr node,
> +                            virResctrlMonitorType tag,
> +                            virDomainResctrlDefPtr resctrl)
> +{
> +    virDomainResctrlMonDefPtr domresmon = NULL;
> +    xmlNodePtr oldnode = ctxt->node;
> +    xmlNodePtr *nodes = NULL;
> +    unsigned int level = 0;
> +    char * tmp = NULL;
> +    char * id = NULL;
> +    size_t i = 0;
> +    int n = 0;
> +    int rv = -1;
> +    int ret = -1;
> +
> +    ctxt->node = node;
> +
> +    if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Cannot extract monitor nodes"));
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < n; i++) {
> +
> +        if (VIR_ALLOC(domresmon) < 0)
> +            goto cleanup;
> +
> +        domresmon->tag = tag;
> +
> +        domresmon->instance = virResctrlMonitorNew();
> +        if (!domresmon->instance) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Could not create monitor"));
> +            goto cleanup;
> +        }
> +
> +        if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> +            tmp = virXMLPropString(nodes[i], "level");
> +            if (!tmp) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("Missing monitor attribute 'level'"));
> +                goto cleanup;
> +            }
> +
> +            if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                               _("Invalid monitor attribute 'level' value '%s'"),
> +                               tmp);
> +                goto cleanup;
> +            }
> +
> +            if (virResctrlMonitorSetCacheLevel(domresmon->instance, level) < 0)
> +                goto cleanup;
> +
> +            VIR_FREE(tmp);
> +        }
> +
> +        if (virDomainResctrlParseVcpus(def, nodes[i], &domresmon->vcpus) < 0)
> +            goto cleanup;
> +
> +        rv = virDomainResctrlMonValidateVcpu(resctrl, domresmon->vcpus);
> +
> +        /* If monitor's vcpu list is identical to allocation's vcpu list,
> +         * set as default monitor */
> +        if (rv == 1 && resctrl->alloc)

I'm still not seeing the need for default... FWIW: The resctrl->alloc
check is unnecessary since the only way rv == 1 is if it's there.

> +            virResctrlMonitorSetDefault(domresmon->instance);
> +        else if (rv < 0)
> +            goto cleanup;
> +
> +        if (!(tmp = virBitmapFormat(domresmon->vcpus)))
> +            goto cleanup;
> +
> +        if (virAsprintf(&id, "vcpus_%s", tmp) < 0)
> +            goto cleanup;
> +
> +        if (virResctrlMonitorSetID(domresmon->instance, id) < 0)
> +            goto cleanup;
> +
> +        if (VIR_APPEND_ELEMENT(resctrl->monitors,
> +                               resctrl->nmonitors,
> +                               domresmon) < 0)
> +            goto cleanup;
> +
> +        VIR_FREE(id);
> +        VIR_FREE(tmp);
> +        domresmon = NULL;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    ctxt->node = oldnode;
> +    VIR_FREE(id);
> +    VIR_FREE(tmp);
> +    virDomainResctrlMonDefFree(domresmon);

     VIR_FREE(nodes);

> +    return ret;
> +}
> +
>  
>  static virDomainResctrlDefPtr
>  virDomainResctrlNew(virResctrlAllocPtr alloc,
> @@ -19041,15 +19206,20 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>          }
>      }
>  
> -    if (virResctrlAllocIsEmpty(alloc)) {
> -        ret = 0;
> -        goto cleanup;
> -    }
> -
>      resctrl = virDomainResctrlNew(alloc, vcpus);

So if @alloc == NULL (which is one of the reasons AllocIsEmpty is true)
means that we'll be virObjectRef(NULL) in ResctrlNew(). In this case
@alloc could be NULL if there were no <cache> entries, IIUC.

I'm starting to see a real downside to a resctrl->alloc == NULL. I
really don't want to continue to see churn on the internal hierarchy
though.

However, if I look at how it could be filled in within the context of
this function, the virDomainResctrlVcpuMatch call and subsequent
possible allocation of @alloc would seemingly be possible outside the
context of whether specific <cache> entries existed. Probably could just
do away with the term default allocation - all it seems to be is an
allocation without <cache> elements, but it can have <monitor> elements.
If someone places a <cachetune> without <cache> and without <monitor>,
so what - who cares. Probably doesn't do much other than limit other
<cachetune> (and perhaps <memorytune>) elements.

>      if (!resctrl)
>          goto cleanup;
>  
> +    if (virDomainResctrlMonDefParse(def, ctxt, node,
> +                                    VIR_RESCTRL_MONITOR_TYPE_CACHE,
> +                                    resctrl) < 0)
> +        goto cleanup;
> +
> +    if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +

Moving the AllocIsEmpty check should be separate.

I'm losing steam, but the next couple of patches had Coverity issues, so
I figured I'll note that before going back to read all the comments
you've posted today while I was reviewing without trying to go back.

John

>      if (virDomainResctrlAppend(def, node, resctrl, flags) < 0)
>          goto cleanup;
>  
> @@ -27085,12 +27255,42 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
>  
>  
>  static int
> +virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon,
> +                                   virResctrlMonitorType tag,
> +                                   virBufferPtr buf)
> +{
> +    char *vcpus = NULL;
> +    unsigned int level = 0;
> +
> +    if (domresmon->tag != tag)
> +        return 0;
> +
> +    virBufferAddLit(buf, "<monitor ");
> +
> +    if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> +        level = virResctrlMonitorGetCacheLevel(domresmon->instance);
> +        virBufferAsprintf(buf, "level='%u' ", level);
> +    }
> +
> +    vcpus = virBitmapFormat(domresmon->vcpus);
> +    if (!vcpus)
> +        return -1;
> +
> +    virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus);
> +
> +    VIR_FREE(vcpus);
> +    return 0;
> +}
> +
> +
> +static int
>  virDomainCachetuneDefFormat(virBufferPtr buf,
>                              virDomainResctrlDefPtr resctrl,
>                              unsigned int flags)
>  {
>      virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
>      char *vcpus = NULL;
> +    size_t i = 0;
>      int ret = -1;
>  
>      virBufferSetChildIndent(&childrenBuf, buf);
> @@ -27099,6 +27299,13 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
>                                      &childrenBuf) < 0)
>          goto cleanup;
>  
> +    for (i = 0; i < resctrl->nmonitors; i ++) {
> +        if (virDomainResctrlMonDefFormatHelper(resctrl->monitors[i],
> +                                               VIR_RESCTRL_MONITOR_TYPE_CACHE,
> +                                               &childrenBuf) < 0)
> +            goto cleanup;
> +    }
> +
>      if (virBufferCheckError(&childrenBuf) < 0)
>          goto cleanup;
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e30a4b2..60f6464 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2236,12 +2236,23 @@ struct _virDomainCputune {
>  };
>  
>  
> +typedef struct _virDomainResctrlMonDef virDomainResctrlMonDef;
> +typedef virDomainResctrlMonDef *virDomainResctrlMonDefPtr;
> +struct _virDomainResctrlMonDef {
> +    virBitmapPtr vcpus;
> +    virResctrlMonitorType tag;
> +    virResctrlMonitorPtr instance;
> +};
> +
>  typedef struct _virDomainResctrlDef virDomainResctrlDef;
>  typedef virDomainResctrlDef *virDomainResctrlDefPtr;
>  
>  struct _virDomainResctrlDef {
>      virBitmapPtr vcpus;
>      virResctrlAllocPtr alloc;
> +
> +    virDomainResctrlMonDefPtr *monitors;
> +    size_t nmonitors;
>  };
>  
>  
> diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml b/tests/genericxml2xmlindata/cachetune-cdp.xml
> index 9718f06..9f4c139 100644
> --- a/tests/genericxml2xmlindata/cachetune-cdp.xml
> +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml
> @@ -8,9 +8,12 @@
>      <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 level='3' vcpus='0'/>
> +      <monitor level='3' vcpus='1'/>
>      </cachetune>
>      <cachetune vcpus='2'>
>        <cache id='1' level='3' type='code' size='6' unit='MiB'/>
> +      <monitor level='3' vcpus='2'/>
>      </cachetune>
>      <cachetune vcpus='3'>
>        <cache id='1' level='3' type='data' size='6912' unit='KiB'/>
> diff --git a/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml
> new file mode 100644
> index 0000000..d481fb5
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml
> @@ -0,0 +1,30 @@
> +<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='768' unit='KiB'/>
> +      <monitor level='3' vcpus='2'/>
> +    </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..748be08 100644
> --- a/tests/genericxml2xmlindata/cachetune-small.xml
> +++ b/tests/genericxml2xmlindata/cachetune-small.xml
> @@ -7,6 +7,13 @@
>    <cputune>
>      <cachetune vcpus='0-1'>
>        <cache id='0' level='3' type='both' size='768' unit='KiB'/>
> +      <monitor level='3' vcpus='0'/>
> +      <monitor level='3' vcpus='1'/>
> +      <monitor level='3' vcpus='0-1'/>
> +    </cachetune>
> +    <cachetune vcpus='2-3'>
> +      <monitor level='3' vcpus='2'/>
> +      <monitor level='3' vcpus='3'/>
>      </cachetune>
>    </cputune>
>    <os>
> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index fa941f0..4393d44 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -137,6 +137,8 @@ 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-monitor", 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);
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux