Re: [PATCH 08/10] conf: introduce resctrl monitor group in domain

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

 




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 @@
>      &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 vcpus='0-1'/&gt;

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.

>      &lt;/cachetune&gt;
>      &lt;memorytune vcpus='0-3'&gt;
>        &lt;node id='0' bandwidth='60'/&gt;
> @@ -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



[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