> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Friday, September 21, 2018 12:16 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: [PATCHv3 1/4] util: Introduce monitor capability interface > > > > On 09/20/2018 06:10 AM, 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 takes the role of RDT monitoring group and could > > be 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> > > Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> > > --- > > src/util/virresctrl.c | 126 > > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 126 insertions(+) > > [...] > > > + * 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; > > + > > + /* For now, monitor only exists in level 3 cache */ > > + info_monitor->cache_level = 3; > > + > > + 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' " > > I changed "path" to "file" to be consistent with he message below. > > > + "does not exist"); > > + ret = 0; > > + virResetLastError(); > > Upon further reflection since it wasn't in the next Uint call -2 failure handling, I > guess this isn't necessary, but I'll leave it just in case something does slip in there > in the future... > > > + 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) { > > + /* 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"); > > I think this should be a VIR_DEBUG, as in is it really that important unless you're > debugging... and although I now think it'd be unnecessary I'll add the > virResetLastError here. > > > + } else 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")); > > + if (rv < 0) > > + goto cleanup; > > + > > + if (!*featurestr) { > > + /* If no feature found in "/info/L3_MON/mon_features", > > + * some error happens */ > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > + _("Get empty feature list from resctrl")); > > + ret = -1; > > The ret = -1; is unnecessary. I think you missed my point. The rv == -2 up a bit > does change it, but it goes to cleanup. I'll remove it. > > I'll fix up things before I push. Just let me know if the VIR_DEBUG is fine or if > you prefer to keep VIR_INFO. It's not that important other than you know you're > going to get the message and seeing it as "INFO" > could alarm someone, so changing to DEBUG means someone could still see the > message and perhaps figure out why the value is 0 in their output instead of > something "real". > VIR_DEBUG is OK for me. Thanks for your review and the hard work! > John > > [...] -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
