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

[John Ferlan <jferlan@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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

> +                allocation. The default monitor has the same vCPU list as the
> +                associating allocation. For non-default monitors, there

associated

> +                are no vCPU overlap permitted.

For non-default monitors, overlapping vCPUs are not permitted.

> +              </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 &&

>          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).

> +
> +    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...

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

John

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