Re: [PATCHv2 1/4] util: Introduce monitor capability interface

[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:39 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 1/4] util: Introduce monitor capability interface
> 
> 
> 
> On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
> > This patch introduces the resource monitor and creates the interface
> > for getting host capability of resource monitor from the system
> > resource control file system.
> >
> > The resource monitor take the role of RDT monitoring group, could be
> 
> *takes...
> 
> s/, could/ and could/
> 
> > used to monitor the resource consumption information, such as the last
> > level cache occupancy and the utilization of memory bandwidth.
> >
> > Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> > ---
> >  src/util/virresctrl.c | 124
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> > 4b5442f..4601f69 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -83,6 +83,9 @@ typedef virResctrlInfoPerLevel
> > *virResctrlInfoPerLevelPtr;  typedef struct _virResctrlInfoMemBW
> > virResctrlInfoMemBW;  typedef virResctrlInfoMemBW
> > *virResctrlInfoMemBWPtr;
> >
> > +typedef struct _virResctrlInfoMongrp virResctrlInfoMongrp; typedef
> > +virResctrlInfoMongrp *virResctrlInfoMongrpPtr;
> > +
> >  typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
> > typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
> >
> > @@ -139,6 +142,28 @@ struct _virResctrlInfoMemBW {
> >      unsigned int max_id;
> >  };
> >
> > +struct _virResctrlInfoMongrp {
> > +    /* 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;
> > +
> > +    /* Last level cache related information */
> > +
> > +    /* 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;
> > +};
> > +
> >  struct _virResctrlInfo {
> >      virObject parent;
> >
> > @@ -146,6 +171,8 @@ struct _virResctrlInfo {
> >      size_t nlevels;
> >
> >      virResctrlInfoMemBWPtr membw_info;
> > +
> > +    virResctrlInfoMongrpPtr monitor_info;
> >  };
> >
> >
> > @@ -171,8 +198,12 @@ virResctrlInfoDispose(void *obj)
> >          VIR_FREE(level);
> >      }
> >
> > +    if (resctrl->monitor_info)
> > +        VIR_FREE(resctrl->monitor_info->features);
> 
> virStringListFree
> 
> > +
> >      VIR_FREE(resctrl->membw_info);
> >      VIR_FREE(resctrl->levels);
> > +    VIR_FREE(resctrl->monitor_info);
> >  }
> >
> >
> > @@ -555,6 +586,92 @@
> > virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)  }
> >
> >
> > +/*
> > + * Retrieve monitor capability from the resource control file system.
> > + *
> > + * The monitor capability is exposed through
> "SYSFS_RESCTRL_PATH/info/L3_MON"
> > + * directory under the resource control file system. The monitor
> > +capability is
> > + * parsed by reading the interface files and stored in the structure
> > + * 'virResctrlInfoMongrp'.
> > + *
> > + * Not all host supports the resource monitor, leave the pointer
> > + * @resctrl->monitor_info empty if not supported.
> > + */
> > +static int
> > +virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) {
> > +    int ret = -1;
> > +    int rv = -1;
> > +    char *featurestr = NULL;
> > +    char **features = NULL;
> > +    size_t nfeatures = 0;
> > +    virResctrlInfoMongrpPtr info_monitor = NULL;
> > +
> > +    if (VIR_ALLOC(info_monitor) < 0)
> > +        return -1;
> > +
> > +    /* monitor only exists in leve 3 cache */
> 
> *level
> 
> Let's say, "For now, monitor only exists in level 3 cache"
> 
> > +    info_monitor->cache_level = 3;
> 
> So I think in the last review I was thinking that if we ever see a different level,
> then the L3 below would need to change. Although for now I wonder if it should
> be removed. I'll leave it unless you really think it should be removed. I think I was
> being ultra careful/paranoid.
> 
> Perhaps the future is GetMonitorInfo takes in the cache_level as a parameter in
> order to build up the path and save the level in a structure.  I think to a degree
> we're good to have that level of indirection with these latest changes.  I don't
> mind removing it completely from this pile, but don't want to mess you up for
> the future either!

I am happy for the change you proposed in last review. Let's keep it here.

> 
> > +
> > +    rv = virFileReadValueUint(&info_monitor->max_monitor,
> > +                              SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
> > +    if (rv == -2) {
> > +        /* The file doesn't exist, so it's unusable for us, probably resource
> > +         * monitor unsupported */
> > +        VIR_INFO("The path '" SYSFS_RESCTRL_PATH
> "/info/L3_MON/num_rmids' "
> > +                 "does not exist");
> 
> Add virResetLastError()
> 
> [avoids having this error in Last and something else failing and spewing the error]
> 
> > +        ret = 0;
> > +        goto cleanup;
> > +    } else if (rv < 0) {
> > +        /* Other failures are fatal, so just quit */
> > +        goto cleanup;
> > +    }
> > +
> > +    rv = virFileReadValueUint(&info_monitor->cache_reuse_threshold,
> > +                              SYSFS_RESCTRL_PATH
> > +                              "/info/L3_MON/max_threshold_occupancy");
> > +    if (rv == -2) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Cannot get max_threshold_occupancy from resctrl"
> > +                         " info"));
> 
> Typically the extra space is on the previous line.  I'm just going to remove the "
> info" completely... Also adding ' ' around the variable name searched on.
> Something I'll repeat for subsequent messages.
> 
> > +    }
> > +    if (rv < 0)
> > +        goto cleanup;
> > +
> > +    rv = virFileReadValueString(&featurestr,
> > +                                SYSFS_RESCTRL_PATH
> > +                                "/info/L3_MON/mon_features");
> > +    if (rv == -2)
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Cannot get mon_features from resctrl info"));
> > +    if (rv < 0)
> > +        goto cleanup;
> > +
> > +    if (!featurestr) {
> 
> I don't think !featurestr could happen as a result of the following in
> virFileReadLimFD (which is is in the call path):
> 
>     s = saferead_lim(fd, maxlen+1, &len);
>     if (s == NULL)
>         return -1;
> ...
>     *buf = s;
>     return len;
>

Agree with your judgement. If should use ' if (!*featurestr)'.
I double confirmed through go through the code as you did and through
testing. Here, if file 'mon_feautres' exists but with empty content, a
buffer will be assigned to 'featurestr' but the first byte will be a
'\0' char. 

> Thus if s = NULL, then we return -1; otherwise, *buf = s or @featurestr = s,
> meaning no chance NULL is set without -1 being returned.
> 
> I think you meant:
> 
>     "if (!*featurestr)"
> 

Yes.

> considering the subsequent lines...
> 
> > +        /* if no feature found in "/info/L3_MON/mon_features",
> > +         * some error happens */
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Cannot get feature list from resctrl
> > + info"));
> 
> Thus this is "Found empty feature list from resctrl"
> 
> > +        ret  = -1;
> 
> This is unnecessary since @ret ==  -1 at this point... @rv could be 0, but we don't
> care. FWIW: one too many spaces between ret and =.
> 

I don't think so.
If code go here, the '/sys/fs/resctrl/info/L3_MON' directory is existed in system,
and further, the  'num_rmids' file is checked that is existed in system, then some
resctrl monitoring feature must be supported, either CMT or MBM. So here the
'mon_features' file shouldn’t be empty. If it is empty, some error happens, report
it.

> 
> I can clean all this up - just let me know that the !*featurestr check is what you
> after....

And I also find a bug that I involved. The fix is shown in following lines.
Since CMT is an independent feature to MBM, some system might not
support CMT while MBM is supported. 

File 'max_threshold_occupancy' is created if CMT is supported, if this
file does not exist, only means CMT might not be supported. We
shouldn't look this as an error.

@@ -632,11 +632,13 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
                               SYSFS_RESCTRL_PATH
                               "/info/L3_MON/max_threshold_occupancy");
     if (rv == -2) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Cannot get max_threshold_occupancy from resctrl"));
-    }
-    if (rv < 0)
+        /* If CMT is not supported, then 'max_threshold_occupancy' file
+         * will not exist. */
+        VIR_INFO("File '" SYSFS_RESCTRL_PATH
+                 "/info/L3_MON/max_threshold_occupancy' does not exist");
+    } else if (rv < 0) {
         goto cleanup;
+    }

     rv = virFileReadValueString(&featurestr,
                                 SYSFS_RESCTRL_PATH

I changed my code according your review opinions , and also, added
the following review message line to my next revised patch version,
I don't know if is proper to do that. 
Thanks for your review.
Huaqiang

> 
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
> 
> John
> 
> > +        goto cleanup;
> > +    }
> > +
> > +    features = virStringSplitCount(featurestr, "\n", 0, &nfeatures);
> > +    VIR_DEBUG("Resctrl supported %ld monitoring features",
> > + nfeatures);
> > +
> > +    info_monitor->nfeatures = nfeatures;
> > +    VIR_STEAL_PTR(info_monitor->features, features);
> > +    VIR_STEAL_PTR(resctrl->monitor_info, info_monitor);
> > +
> > +    ret = 0;
> > + cleanup:
> > +    VIR_FREE(featurestr);
> > +    virStringListFree(features);
> > +    VIR_FREE(info_monitor);
> > +    return ret;
> > +}
> > +
> > +
> >  static int
> >  virResctrlGetInfo(virResctrlInfoPtr resctrl)  { @@ -573,6 +690,10 @@
> > virResctrlGetInfo(virResctrlInfoPtr resctrl)
> >      if (ret < 0)
> >          goto cleanup;
> >
> > +    ret = virResctrlGetMonitorInfo(resctrl);
> > +    if (ret < 0)
> > +        goto cleanup;
> > +
> >      ret = 0;
> >   cleanup:
> >      VIR_DIR_CLOSE(dirp);
> > @@ -613,6 +734,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
> >      if (resctrl->membw_info)
> >          return false;
> >
> > +    if (resctrl->monitor_info)
> > +        return false;
> > +
> >      for (i = 0; i < resctrl->nlevels; i++) {
> >          virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
> >
> >

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