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

[Huaqiang,Wang]

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 at 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
> 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 at redhat.com>
>
> John

Thanks for fixing and the review, I will make changes in my code.

Huaqiang