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

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

 




> -----Original Message-----
> From: John Ferlan [mailto:jferlan@xxxxxxxxxx]
> Sent: Wednesday, September 19, 2018 3:47 AM
> To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx
> Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>;
> Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx>
> Subject: Re: [PATCHv2 4/4] conf: Introduce RDT monitor host capability
> 
> 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_thresh
> > old_occupancy  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-cmt/resctrl/info/L3_MON/mon_featur
> > es  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_m
> > ask  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/min_c
> > bm_bits  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3/num_c
> > losids  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/m
> > ax_threshold_occupancy  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/m
> > on_features  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/L3_MON/n
> > um_rmids  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/bandw
> > idth_gran  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/min_b
> > andwidth  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/info/MB/num_c
> > losids  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/cpu
> > s  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/sch
> > emata  create mode 100644
> > tests/vircaps2xmldata/linux-resctrl-fake-feature/resctrl/manualres/tas
> > ks  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...

Embarrassing...
Will be fixed.

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

Will be fixed.

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

Will be fixed.

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

Yes, mon->nfeatures will not be 0, it is ensured in time of creating the
*monitor. But we should still keep the two lines here to prevent the emergence
of
        <monitor maxMonitors='127'/>

"<monitor maxMonitors='127'/>" is invalid from the perspective of any
existing hardware. I can not image the use of a resource monitor without a
counter (indicating through feature name ) associating with it.

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

Will be fixed.

> 
> > +
> > +    for (i = 0; i < monitor->nfeatures; i++) {
> > +        virBufferSetChildIndent(&childrenBuf, buf);
> 
> This can be to put this outside the for loop

Thanks, will be moved outside.

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

You mean change it to

if (virCapabilitiesFormatResctrlMonitor(buf, cache->monitor) < 0)
right?

Will be fixed.

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

Will be fixed.

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

Will be fixed.

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

Will be fxied.

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

OK.

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

Will be fixed.

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

Let's remove it as well as that 'VIR_INFO' line.

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

Agree.

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

It is swapped by unknown reason, and I don't need any kind of
specific order, let's use the original order.

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

Will be fixed.

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

Will be fixed.

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

No, no empty feature list will be returned. If feature list for monitor
with @prefix is empty, @monitor will be set to NULL, and 0 will will
be returned. This tells caller the monitor with @prefix is not supported
in system and which is a tolerable case.

This will be guaranteed by performing following line
     /* In case *monitor is pointed to some monitor, clean it. */
        virResctrlInfoMonFree(*monitor);
This line will be moved like this, to ensure *monitor=NULL if mon->nfeatures == 0
        
             ret = 0;
        
        +    /* In case *monitor is pointed to some monitor, clean it. */
        +    virResctrlInfoMonFree(*monitor);
        +
             if (mon->nfeatures == 0) {
                 /* No feature found for current monitor, means host does not support
                  * monitor type with @prefix name.
        @@ -959,11 +957,8 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
                 goto cleanup;
             }
        
        -    /* In case *monitor is pointed to some monitor, clean it. */
        -    VIR_FREE(*monitor);
        -
             VIR_STEAL_PTR(*monitor, mon);
        

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

Will be fixed.

New notation for this function is like this:

         /* virResctrlInfoGetMonitorPrefix
          *
          * @resctrl: Pointer to virResctrlInfo
          * @prefix: Monitor prefix name for monitor looking for.
          * @monitor: Returns the capability information for target monitor if the
          * monitor with @prefex is supported by host.
          *
          * Return monitor capability information for @prefix through @monitor.
          * If monitor with @prefix is not supported in system, @monitor will be
          * cleared to NULL.
          *
          * Returns 0 if @monitor is created or monitor type with @prefix is not
          * supported by host, -1 on failure with error message set.
          */

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

Moved before line
    if (virResctrlInfoIsEmpty(resctrl))

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

Can we use enum type data structure? It is quite common in libvirt.
It should be ok for using array and ARRAY_CARDINALITY, but need to change some
code.

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

OK.

> 
> > +                       prefix);
> > +        goto cleanup;
> 
> could be just return -1 since @mon == NULL and that's all cleanup does is clean
> it up.

Yes, I think it could return -1 directly here.

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

Will be fixed.

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

Just tell user some resctrl monitoring is not supported by your system.

> 
> Also, since I think @virMonitor is overkill, let's just go with:
> 
>     VIR_INFO("No resctrl monitor features using prefix '%s' found",
>              prefix);

Will remove VIR_ENUM_IMPL(virMonitor...) and will change as you recommended.

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

return 0, and clear *monitor to NULL, means monitor with @prefix is not
supported here. caller should continue to check next type monitor.

> 
> > +    }
> > +
> > +    /* In case *monitor is pointed to some monitor, clean it. */
> > +    VIR_FREE(*monitor);
> 
> This should be virResctrlInfoMonFree, right?

Good catch. Will be fixed.


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

Your naming is better. Thanks.

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

Will be fixed.

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

Seems I said too much. Let's use an more precise way:

        struct _virResctrlInfoMon {
            /* 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 */
            virResctrlMonitorType type;
            /* 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;
        };


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

Yes. CMT, MBM as well as CAT, MBA are all independent hardware features, we
may encounter some system only support CMT, this test is designed for
this case.

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

Thank you for your review.
I'll also submit the patches fixed, just for your reference.
Huaqiang

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