Re: [PATCHv2 4/4] conf: Introduce RDT monitor host capability

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

 



ca

On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
> Cache monitoring technology(CMT) and memory bandwidth
> monitoring technology(MBM) are resource monitoring
> part of Intel resource director technology(RDT).
> 
> This patch is introducing cache monitor(CMT) to cache and
> memory bandwidth monitor(MBM) for monitoring CPU memory
> bandwidth.

There's some redundancy in the above...

> The host capability of the two monitors is also introduced
> in this patch.

The above two paragraphs can be collapsed to:

This patch introduces CMT and MBM for monitoring CPU cache
and memory bandwidth into the host capability XML output.

> 
> For cache monitor, the host capability is shown like:

For CMT, ...

>   <host>
>   ...
>     <cache>
>       <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'>
>         <control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/>
>       </bank>
>       <monitor level='3' 'reuseThreshold'='270336' maxMonitors='176'>
>         <feature name='llc_occupancy'/>
>       </monitor>
>     </cache>
>     ...
>   </host>
> 
> For memory bandwidth monitor, the capability is shown like this:

For MBM, ...

> 
>   <host>
>     ...
>     <memory_bandwidth>
>       <node id='1' cpus='6-11'>
>         <control granularity='10' min ='10' maxAllocs='4'/>
>       </node>
>       <monitor maxMonitors='176'>
>         <feature name='mbm_total_bytes'/>
>         <feature name='mbm_local_bytes'/>
>       </monitor>
>     </memory_bandwidth>
>     ...
>   </host>
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> ---
>  docs/schemas/capability.rng                        |  37 ++++++-
>  src/conf/capabilities.c                            |  68 ++++++++++++
>  src/conf/capabilities.h                            |   4 +
>  src/libvirt_private.syms                           |   2 +
>  src/util/virresctrl.c                              | 120 ++++++++++++++++++++-
>  src/util/virresctrl.h                              |  62 +++++++++++
>  .../resctrl/info/L3_MON/max_threshold_occupancy    |   1 +
>  .../resctrl/info/L3_MON/mon_features               |   1 +
>  .../resctrl/info/L3_MON/num_rmids                  |   1 +
>  .../linux-resctrl-cmt/resctrl/manualres/cpus       |   1 +
>  .../linux-resctrl-cmt/resctrl/manualres/schemata   |   1 +
>  .../linux-resctrl-cmt/resctrl/manualres/tasks      |   0
>  .../linux-resctrl-cmt/resctrl/schemata             |   1 +
>  tests/vircaps2xmldata/linux-resctrl-cmt/system     |   1 +
>  .../resctrl/info/L3/cbm_mask                       |   1 +
>  .../resctrl/info/L3/min_cbm_bits                   |   1 +
>  .../resctrl/info/L3/num_closids                    |   1 +
>  .../resctrl/info/L3_MON/max_threshold_occupancy    |   1 +
>  .../resctrl/info/L3_MON/mon_features               |  10 ++
>  .../resctrl/info/L3_MON/num_rmids                  |   1 +
>  .../resctrl/info/MB/bandwidth_gran                 |   1 +
>  .../resctrl/info/MB/min_bandwidth                  |   1 +
>  .../resctrl/info/MB/num_closids                    |   1 +
>  .../resctrl/manualres/cpus                         |   1 +
>  .../resctrl/manualres/schemata                     |   1 +
>  .../resctrl/manualres/tasks                        |   0
>  .../linux-resctrl-fake-feature/resctrl/schemata    |   1 +
>  .../linux-resctrl-fake-feature/system              |   1 +
>  .../resctrl/info/L3_MON/max_threshold_occupancy    |   1 +
>  .../linux-resctrl/resctrl/info/L3_MON/mon_features |   3 +
>  .../linux-resctrl/resctrl/info/L3_MON/num_rmids    |   1 +
>  .../vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml |  53 +++++++++
>  .../vircaps-x86_64-resctrl-fake-feature.xml        |  73 +++++++++++++
>  tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml   |   7 ++
>  tests/vircaps2xmltest.c                            |   2 +
>  35 files changed, 459 insertions(+), 3 deletions(-)
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/max_threshold_occupancy
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_features
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/num_rmids
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/cpus
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/schemata
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/manualres/tasks
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/schemata
>  create mode 120000 tests/vircaps2xmldata/linux-resctrl-cmt/system
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/cbm_mask
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_cbm_bits
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_closids
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/max_threshold_occupancy
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/mon_features
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/num_rmids
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandwidth_gran
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_bandwidth
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_closids
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpus
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/schemata
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tasks
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/schemata
>  create mode 120000 tests/vircaps2xmldata/linux-resctrl-fake-feature/system
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features
>  create mode 100644 tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids
>  create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cmt.xml
>  create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-fake-feature.xml
> 
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index d61515c..fe12bf9 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -316,6 +316,9 @@
>            </zeroOrMore>
>          </element>
>        </oneOrMore>
> +      <optional>
> +        <ref name='cpuMonitor'/>
> +      </optional>
>      </element>
>    </define>
>  
> @@ -347,7 +350,7 @@
>              <optional>
>                <attribute name='min'>
>                  <ref name='unsignedInt'/>
> -            </attribute>
> +              </attribute>
>              </optional>
>                <attribute name='maxAllocs'>
>                  <ref name='unsignedInt'/>
> @@ -356,9 +359,41 @@
>            </zeroOrMore>
>          </element>
>        </oneOrMore>
> +      <optional>
> +        <ref name='cpuMonitor'/>
> +      </optional>
>      </element>
>    </define>
>  
> +  <define name='cpuMonitor'>
> +    <element name='monitor'>
> +      <optional>
> +        <attribute name='level'>
> +          <ref name='unsignedInt'/>
> +        </attribute>
> +        <attribute name='reuseThreshold'>
> +          <ref name='unsignedInt'/>
> +        </attribute>
> +        </optional>

Ironically you fixed alignment above, but created more misalignment.
I've cleaned it up...

> +        <attribute name='maxMonitors'>
> +          <ref name='unsignedInt'/>
> +        </attribute>
> +        <oneOrMore>
> +          <element name='feature'>
> +            <attribute name='name'>
> +              <ref name='monitorFeature'/>
> +            </attribute>
> +          </element>
> +      </oneOrMore>
> +    </element>
> +  </define>
> +
> +  <define name='monitorFeature'>
> +    <data type='string'>
> +      <param name='pattern'>(llc_|mbm_)[a-zA-Z0-9\-_]+</param>
> +    </data>
> +  </define>
> +
>    <define name='guestcaps'>
>      <element name='guest'>
>        <ref name='ostype'/>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 66ad420..ba69d6f 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -247,10 +247,12 @@ virCapsDispose(void *object)
>  
>      for (i = 0; i < caps->host.cache.nbanks; i++)
>          virCapsHostCacheBankFree(caps->host.cache.banks[i]);
> +    virResctrlInfoMonFree(caps->host.cache.monitor);
>      VIR_FREE(caps->host.cache.banks);
>  
>      for (i = 0; i < caps->host.memBW.nnodes; i++)
>          virCapsHostMemBWNodeFree(caps->host.memBW.nodes[i]);
> +    virResctrlInfoMonFree(caps->host.memBW.monitor);
>      VIR_FREE(caps->host.memBW.nodes);
>  
>      VIR_FREE(caps->host.netprefix);
> @@ -867,6 +869,50 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
>  }
>  

Two blank lines

>  static int
> +virCapabilitiesFormatResctrlMonitor(virBufferPtr buf,
> +                                    virResctrlInfoMonPtr monitor)
> +{
> +    size_t i = 0;
> +    virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> +
> +    /* monitor not supported, no capability */
> +    if (!monitor)
> +        return 0;
> +
> +    /* no feature found in mointor means no capability, return */

*monitor

> +    if (monitor->nfeatures == 0)
> +        return 0;

I don't believe we can get the above. See [1] in
virResctrlInfoGetMonitorPrefix - we never steal @mon for @*monitor when
mon->nfeatures == 0

I can leave it for safety, unless you think it would ever be useful to
display:

    <monitor maxMonitors='127'/>

In which case I'll move the == 0 check below, slap a "/>" on and return
0 - ignoring childrenBuf

> +
> +    virBufferAddLit(buf, "<monitor ");
> +
> +    /* CMT might not enabled, if enabled show related attributes. */
> +    if (monitor->type == VIR_MONITOR_TYPE_CACHE)
> +        virBufferAsprintf(buf,
> +                          "level='%u' reuseThreshold='%u' ",
> +                          monitor->cache_level,
> +                          monitor->cache_reuse_threshold);
> +    virBufferAsprintf(buf,
> +                      "maxMonitors='%u'>\n",
> +                      monitor->max_monitor);
> +

One less blank line here.

> +
> +    for (i = 0; i < monitor->nfeatures; i++) {
> +        virBufferSetChildIndent(&childrenBuf, buf);

This can be to put this outside the for loop

> +        virBufferAsprintf(&childrenBuf,
> +                          "<feature name='%s'/>\n",
> +                          monitor->features[i]);
> +    }
> +
> +    if (virBufferCheckError(&childrenBuf) < 0)
> +        return -1;
> +
> +    virBufferAddBuffer(buf, &childrenBuf);
> +    virBufferAddLit(buf, "</monitor>\n");
> +
> +    return 0;
> +}
> +
> +static int
>  virCapabilitiesFormatCaches(virBufferPtr buf,
>                              virCapsHostCachePtr cache)
>  {
> @@ -953,6 +999,10 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>          }
>      }
>  
> +    if (virCapabilitiesFormatResctrlMonitor(buf,
> +                                            cache->monitor) < 0)

Fits on previous line, I moved it.

> +        return -1;
> +
>      virBufferAdjustIndent(buf, -2);
>      virBufferAddLit(buf, "</cache>\n");
>  
> @@ -1004,6 +1054,10 @@ virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf,
>          }
>      }
>  
> +    if (virCapabilitiesFormatResctrlMonitor(buf,
> +                                            memBW->monitor) < 0)

Same here.

> +        return -1;
> +
>      virBufferAdjustIndent(buf, -2);
>      virBufferAddLit(buf, "</memory_bandwidth>\n");
>  
> @@ -1664,6 +1718,8 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps)
>      size_t i = 0;
>      int ret = -1;
>  

Removed blank line here.

> +    const char *prefix = virMonitorPrefixTypeToString(VIR_MONITOR_TYPE_MEMBW);
> +
>      for (i = 0; i < caps->host.cache.nbanks; i++) {
>          virCapsHostCacheBankPtr bank = caps->host.cache.banks[i];
>          if (VIR_ALLOC(node) < 0)
> @@ -1684,6 +1740,11 @@ virCapabilitiesInitResctrlMemory(virCapsPtr caps)
>          node = NULL;
>      }
>  
> +    if (virResctrlInfoGetMonitorPrefix(caps->host.resctrl,
> +                                       prefix,

Fits on previous line

> +                                       &caps->host.memBW.monitor) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virCapsHostMemBWNodeFree(node);
> @@ -1704,6 +1765,8 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>      struct dirent *ent = NULL;
>      virCapsHostCacheBankPtr bank = NULL;
>  

Same - removed blank line

> +    const char *prefix = virMonitorPrefixTypeToString(VIR_MONITOR_TYPE_CACHE);
> +
>      /* Minimum level to expose in capabilities.  Can be lowered or removed (with
>       * the appropriate code below), but should not be increased, because we'd
>       * lose information. */
> @@ -1823,6 +1886,11 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>      if (virCapabilitiesInitResctrlMemory(caps) < 0)
>          goto cleanup;
>  
> +    if (virResctrlInfoGetMonitorPrefix(caps->host.resctrl,
> +                                       prefix,

Fits on previous line.

> +                                       &caps->host.cache.monitor) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      VIR_FREE(type);
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 694fd6b..45b331a 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -156,6 +156,8 @@ typedef virCapsHostCache *virCapsHostCachePtr;
>  struct _virCapsHostCache {
>      size_t nbanks;
>      virCapsHostCacheBankPtr *banks;
> +
> +    virResctrlInfoMonPtr monitor;
>  };
>  
>  typedef struct _virCapsHostMemBWNode virCapsHostMemBWNode;
> @@ -171,6 +173,8 @@ typedef virCapsHostMemBW *virCapsHostMemBWPtr;
>  struct _virCapsHostMemBW {
>      size_t nnodes;
>      virCapsHostMemBWNodePtr *nodes;
> +
> +    virResctrlInfoMonPtr monitor;
>  };
>  
>  typedef struct _virCapsHost virCapsHost;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e7a4d61..8061e1c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2668,6 +2668,8 @@ virResctrlAllocSetCacheSize;
>  virResctrlAllocSetID;
>  virResctrlAllocSetMemoryBandwidth;
>  virResctrlInfoGetCache;
> +virResctrlInfoGetMonitorPrefix;
> +virResctrlInfoMonFree;
>  virResctrlInfoNew;
>  
>  
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 4601f69..156618f 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -70,6 +70,18 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST,
>                "CODE",
>                "DATA")
>  
> +/* Monitor name mapping for monitor naming */
> +VIR_ENUM_IMPL(virMonitor, VIR_MONITOR_TYPE_LAST,
> +              "unsupported monitor",
> +              "cache monitor",
> +              "memory bandwidth monitor")

This is probably overkill for the usage in a VIR_INFO, I've removed it
and you'll see my suggestion for VIR_INFO below.

> +
> +/* Monitor feature name prefix mapping for monitor naming */
> +VIR_ENUM_IMPL(virMonitorPrefix, VIR_MONITOR_TYPE_LAST,

Should be "virResctrlMonitorPrefix"

> +              "__unsupported__",
> +              "llc_",
> +              "mbm_")
> +
>  
>  /* All private typedefs so that they exist for all later definitions.  This way
>   * structs can be included in one or another without reorganizing the code every
> @@ -207,6 +219,17 @@ virResctrlInfoDispose(void *obj)
>  }
>  
>  
> +void
> +virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
> +{
> +    if (!mon)
> +        return;
> +
> +    virStringListFree(mon->features);
> +    VIR_FREE(mon);
> +}
> +
> +
>  /* virResctrlAlloc */
>  
>  /*
> @@ -686,11 +709,11 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>      if (ret < 0)
>          goto cleanup;
>  
> -    ret = virResctrlGetCacheInfo(resctrl, dirp);
> +    ret = virResctrlGetMonitorInfo(resctrl);
>      if (ret < 0)
>          goto cleanup;
>  
> -    ret = virResctrlGetMonitorInfo(resctrl);
> +    ret = virResctrlGetCacheInfo(resctrl, dirp);

Why did the order switch?

If they need to be this order, then patch1 needs adjustment.

I've changed back to the former order for now.

>      if (ret < 0)
>          goto cleanup;
>  
> @@ -851,6 +874,99 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
>  }
>  
>  
> +/* virResctrlInfoGetMonitorPrefix
> + *
> + * @resctrl: Pointer to virResctrlInfo
> + * @prefix: Monitor prefix name for monitor looking for.
> + * @monitor: Returns the capability inforamtion for taget monitor if the

*information
*target

> + * monitor with prefix name @prefex is supported by host.

s/prefix name @prefex/@prefix

> + *
> + * Return monitor capability information describe in prefix name @prefix
> + * through @monitor

 * Return monitor capability information for @prefix through @monitor.
 * It is possible to return an empty list of features for @monitor
 * leaving it up to the caller to handle.

> + *
> + * Returns 0 if a monitor is found or a valid monitor is not supported by host,
> + * -1 on failure with error message set.
> + * */

s/* */*/

> +int
> +virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
> +                               const char *prefix,
> +                               virResctrlInfoMonPtr *monitor)
> +{
> +    size_t i = 0;
> +    virResctrlInfoMongrpPtr mongrp_info = NULL;
> +    virResctrlInfoMonPtr mon = NULL;
> +    int ret = -1;
> +
> +    if (virResctrlInfoIsEmpty(resctrl))
> +        return 0;
> +
> +    mongrp_info = resctrl->monitor_info;
> +
> +    if (!mongrp_info) {
> +        VIR_INFO("Monitor is not supported in host");
> +        return 0;
> +    }
> +
> +    if (!prefix) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Empty prefix name for resctrl monitor"));
> +        return -1;
> +    }

Could be earlier, but it whatever.

> +
> +    for (i = 0; i < VIR_MONITOR_TYPE_LAST; i++) {
> +        if (STREQ(prefix, virMonitorPrefixTypeToString(i))) {

I'm sure there's a way with ARRAY_CARDINALITY too - that's something I
haven't quite figured out.

> +            if (VIR_ALLOC(mon) < 0)
> +                goto cleanup;
> +            mon->type = i;
> +            break;
> +        }
> +    }
> +
> +    if (!mon) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Bad prefix name \"%s\" for resctrl monitor"),

use '%s'

> +                       prefix);
> +        goto cleanup;

could be just return -1 since @mon == NULL and that's all cleanup does
is clean it up.

> +    }
> +
> +    mon->max_monitor = mongrp_info->max_monitor;
> +
> +    if (mon->type == VIR_MONITOR_TYPE_CACHE) {
> +        mon->cache_reuse_threshold =  mongrp_info->cache_reuse_threshold;
> +        mon->cache_level =  mongrp_info->cache_level;

s/=  /= /

Just one space.

> +    }
> +
> +    for (i = 0; i < mongrp_info->nfeatures; i++) {
> +        if (STRPREFIX(mongrp_info->features[i], prefix)) {
> +            if (virStringListAdd(&mon->features,
> +                                 mongrp_info->features[i]) < 0)
> +                goto cleanup;
> +            mon->nfeatures++;
> +        }
> +    }
> +
> +    ret = 0;
> +
> +    if (mon->nfeatures == 0) {
> +        /* No feature found for current monitor, means host does not support
> +         * monitor type with @prefix name.
> +         * Telling caller this monitor is supported by hardware specification,
> +         * but not supported by this host */
> +        VIR_INFO("%s is not supported by host",
> +                 virMonitorTypeToString(mon->type));

Actually this just logs a message (if we're printing VIR_INFO) on the
daemon side. The consumer on the other end is goint to have to "handle"
nfeatures == 0 then.

Also, since I think @virMonitor is overkill, let's just go with:

    VIR_INFO("No resctrl monitor features using prefix '%s' found",
             prefix);


> +        goto cleanup;

[1] If we go to cleanup, then we return 0, but never steal @mon. Our
"consumer" that does the Format won't ever get @*monitor filled in.

> +    }
> +
> +    /* In case *monitor is pointed to some monitor, clean it. */
> +    VIR_FREE(*monitor);

This should be virResctrlInfoMonFree, right?

> +
> +    VIR_STEAL_PTR(*monitor, mon);
> + cleanup:
> +    virResctrlInfoMonFree(mon);
> +    return ret;
> +}
> +
> +
>  /* virResctrlAlloc-related definitions */
>  virResctrlAllocPtr
>  virResctrlAllocNew(void)
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index cfd56dd..949e20f 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -36,6 +36,16 @@ typedef enum {
>  VIR_ENUM_DECL(virCache);
>  VIR_ENUM_DECL(virCacheKernel);
>  

The following are missing the Resctrl or RESCRTL_ name "prefix"... e.g.
virResctrlMonitor* and VIR_RESCTRL_MONITOR*

I'll adjust...

> +typedef enum {
> +    VIR_MONITOR_TYPE_UNSUPPORT,
> +    VIR_MONITOR_TYPE_CACHE,
> +    VIR_MONITOR_TYPE_MEMBW,
> +
> +    VIR_MONITOR_TYPE_LAST
> +} virMonitorType;
> +VIR_ENUM_DECL(virMonitor);

No use for ^^ this one.

> +VIR_ENUM_DECL(virMonitorPrefix);
> +
>  
>  typedef struct _virResctrlInfoPerCache virResctrlInfoPerCache;
>  typedef virResctrlInfoPerCache *virResctrlInfoPerCachePtr;
> @@ -61,6 +71,51 @@ struct _virResctrlInfoMemBWPerNode {
>      unsigned int max_allocation;
>  };
>  
> +typedef struct _virResctrlInfoMon virResctrlInfoMon;
> +typedef virResctrlInfoMon *virResctrlInfoMonPtr;
> +struct _virResctrlInfoMon {
> +    /* Common fields */
> +
> +    /* Maximum number of simultaneous monitors */
> +    unsigned int max_monitor;
> +    /* null-terminal string list for monitor features */
> +    char **features;
> +    /* Number of monitor features */
> +    size_t nfeatures;
> +    /* Monitor type */
> +    virMonitorType type;

virResctrlMonitorType type;

I'm not a total fan of just 'type', but there's precedent so I won't
change it.  Finding "->type " in the code can be like looking a needle
in the haystack.

> +
> +    /* cache monitor (CMT) related field
> +     *
> +     * CMT has following resource monitor event:
> +     *      "llc_occupancy"
> +     *
> +     * If this is a cache monitor, the memory bandwidth monitor related
> +     * fields and feture events will not be valid. */

*feature

Hmmm.. I'm not really sure how to read the above. Is it "important" for
the consumer to know the relationship to MBM from a data fetching viewpoint?

> +
> +    /* This Adjustable value affects the final reuse of resources used by
> +     * monitor. After the action of removing a monitor, the kernel may not
> +     * release all hardware resources that monitor used immediately if the
> +     * cache occupancy value associated with 'removed' monitor is above this
> +     * threshold. Once the cache occupancy is below this threshold, the
> +     * underlying hardware resource will be reclaimed and be put into the
> +     * resource pool for next reusing.*/
> +    unsigned int cache_reuse_threshold;
> +    /* The cache 'level' that has the monitor capability */
> +    unsigned int cache_level;
> +
> +    /* memory bandwidth monitor (MBM) related field
> +     *
> +     * MBM has following resource monitor events:
> +     *      "mbm_total_bytes"
> +     *      "mbm_local_bytes"
> +     *
> +     * If this is a memory bandwidth monitor, the cache monitor related
> +     * fields and feature events will not be valid. */

Similar type, but opposite question - is this an implementation detail
that the normal consumer really won't have to know.

> +
> +    /* MBM related field is empty */

Kind of an odd comment - not sure what to do with it or think about it.

> +};
> +
>  typedef struct _virResctrlInfo virResctrlInfo;
>  typedef virResctrlInfo *virResctrlInfoPtr;
>  
> @@ -145,4 +200,11 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
>  int
>  virResctrlAllocRemove(virResctrlAllocPtr alloc);
>  
> +void
> +virResctrlInfoMonFree(virResctrlInfoMonPtr mon);
> +
> +int
> +virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
> +                               const char *prefix,
> +                               virResctrlInfoMonPtr *monitor);

Use ATTRIBUTE_NONNULL(3)... The will *Free it and replace it so if what
@*monitor points at is NULL, that's fine, but if it's NULL, then my
coverity build will capture that.

>  #endif /*  __VIR_RESCTRL_H__ */

[...]

> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> index 1fb8861..7f3c26a 100644
> --- a/tests/vircaps2xmltest.c
> +++ b/tests/vircaps2xmltest.c
> @@ -111,9 +111,11 @@ mymain(void)
>      DO_TEST_FULL("caches", VIR_ARCH_X86_64, true, true);
>  
>      DO_TEST_FULL("resctrl", VIR_ARCH_X86_64, true, true);
> +    DO_TEST_FULL("resctrl-cmt", VIR_ARCH_X86_64, true, true);

So this one just shows that <memory_bandwidth> may not be displayed,
true? And of course the <control> is optional too for <bank>.

I can make all the suggested alterations with your OK. I think there
were a couple of questions about things which I think we can easily
hammer out.

John


>      DO_TEST_FULL("resctrl-cdp", VIR_ARCH_X86_64, true, true);
>      DO_TEST_FULL("resctrl-skx", VIR_ARCH_X86_64, true, true);
>      DO_TEST_FULL("resctrl-skx-twocaches", VIR_ARCH_X86_64, true, true);
> +    DO_TEST_FULL("resctrl-fake-feature", VIR_ARCH_X86_64, true, true);
>  
>      return ret;
>  }
> 

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