Re: [libvirt] [PATCH 12/21] resctrl: Instantiate all resctrl information at once

On 11/13/2017 03:50 AM, Martin Kletzander wrote: > This allows for looking up the cache control information more sensibly from > conf/capabilities.c and also provides more data to the virresctrl module that > will get more usable later on. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > po/POTFILES.in | 1 + > src/conf/capabilities.c | 48 +++---- > src/conf/capabilities.h | 4 +- > src/libvirt_private.syms | 4 +- > src/util/virresctrl.c | 335 +++++++++++++++++++++++++++++++++-------------- > src/util/virresctrl.h | 24 ++-- > 6 files changed, 274 insertions(+), 142 deletions(-) > [...] > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c > index 2a11825a52dc..6c6692e78f42 100644 > --- a/src/util/virresctrl.c > +++ b/src/util/virresctrl.c > @@ -18,6 +18,11 @@ > > #include <config.h> > > +#include <sys/file.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <fcntl.h> > + > #include "virresctrl.h" > > #include "c-ctype.h" > @@ -25,12 +30,16 @@ > #include "viralloc.h" > #include "virfile.h" > #include "virlog.h" > +#include "virobject.h" > #include "virstring.h" > > + > #define VIR_FROM_THIS VIR_FROM_RESCTRL > > VIR_LOG_INIT("util.virresctrl") > > + > +/* Common definitions */ > #define SYSFS_RESCTRL_PATH "/sys/fs/resctrl" > > /* Resctrl is short for Resource Control. It might be implemented for various > @@ -55,133 +64,257 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST, > "CODE", > "DATA") > > -int > -virResctrlGetCacheInfo(unsigned int level, > - unsigned long long size, > - virCacheType scope, > - virResctrlInfoPtr **controls, > - size_t *ncontrols) > -{ > - int ret = -1; > - char *tmp = NULL; > - char *path = NULL; > - char *cbm_mask = NULL; > - char *type_upper = NULL; > - unsigned int bits = 0; > - unsigned int min_cbm_bits = 0; > - virResctrlInfoPtr control; > - > - if (VIR_ALLOC(control) < 0) > - goto cleanup; > > - if (scope != VIR_CACHE_TYPE_BOTH && > - virStringToUpper(&type_upper, virCacheTypeToString(scope)) < 0) > - goto cleanup; > +/* Info-related definitions and InfoClass-related functions */ > +typedef struct _virResctrlInfoPerType virResctrlInfoPerType; > +typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr; > +struct _virResctrlInfoPerType { > + /* Kernel-provided information */ > + char *cbm_mask; > + unsigned int min_cbm_bits; > > - if (virFileReadValueUint(&control->max_allocation, > - SYSFS_RESCTRL_PATH "/info/L%u%s/num_closids", > - level, > - type_upper ? type_upper : "") < 0) > - goto cleanup; > + /* Our computed information from the above */ > + unsigned int bits; > + unsigned int max_cache_id; > > - if (virFileReadValueString(&cbm_mask, > - SYSFS_RESCTRL_PATH > - "/info/L%u%s/cbm_mask", > - level, > - type_upper ? type_upper: "") < 0) > - goto cleanup; > + /* In order to be self-sufficient we need size information per cache. > + * Funnily enough, one of the outcomes of the resctrlfs design is that it > + * does not account for different sizes per cache on the same level. So > + * for the sake of easiness, let's copy that, for now. */ > + unsigned long long size; > > - if (virFileReadValueUint(&min_cbm_bits, > - SYSFS_RESCTRL_PATH "/info/L%u%s/min_cbm_bits", > - level, > - type_upper ? type_upper : "") < 0) > - goto cleanup; > + /* Information that we will return upon request (this is public struct) as > + * until now all the above is internal to this module */ > + virResctrlInfoPerCache control; > +}; > > - virStringTrimOptionalNewline(cbm_mask); > +typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel; > +typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr; > +struct _virResctrlInfoPerLevel { > + virResctrlInfoPerTypePtr *types; > +}; > > - for (tmp = cbm_mask; *tmp != '\0'; tmp++) { > - if (c_isxdigit(*tmp)) > - bits += count_one_bits(virHexToBin(*tmp)); > - } > +struct _virResctrlInfo { > + virObject parent; > > - control->granularity = size / bits; > - if (min_cbm_bits != 1) > - control->min = min_cbm_bits * control->granularity; > + virResctrlInfoPerLevelPtr *levels; > + size_t nlevels; > +}; > > - control->scope = scope; > +static virClassPtr virResctrlInfoClass; > > - if (VIR_APPEND_ELEMENT(*controls, *ncontrols, control) < 0) > - goto cleanup; > +static void > +virResctrlInfoDispose(void *obj) > +{ > + size_t i = 0; > + size_t j = 0; > > - ret = 0; > + virResctrlInfoPtr resctrl = obj; > > - cleanup: > - VIR_FREE(path); > - VIR_FREE(cbm_mask); > - VIR_FREE(type_upper); > - VIR_FREE(control); > - return ret; > -} > + for (i = 0; i < resctrl->nlevels; i++) { > + virResctrlInfoPerLevelPtr level = resctrl->levels[--resctrl->nlevels]; This decrements the for end loop condition... So this code is going backwards through the levels... I guess it works, but it's one of those "things" about altering the ending of a for loop within the for loop that just "looks strange"... Alternatively: while ((level = resctrl->levels[--resctrl->nlevels])) right? Nothing that I'd ask to change - it just "looks strange"...

> + > + if (!level) > + continue; > > + if (level->types) { > + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) level->types[j]->cbm_mask is leaked.

> + VIR_FREE(level->types[j]); > + } > + VIR_FREE(level->types); > + VIR_FREE(level); > + } > + > + VIR_FREE(resctrl->levels); > +} > > -static inline int > -virResctrlGetCacheDir(char **path, > - const char *prefix, > - unsigned int level, > - virCacheType type) > +static int virResctrlInfoOnceInit(void) > { > - return virAsprintf(path, > - SYSFS_RESCTRL_PATH "%s/L%u%s", > - prefix ? prefix : "", > - level, > - virResctrlTypeToString(type)); > + if (!(virResctrlInfoClass = virClassNew(virClassForObject(), > + "virResctrlInfo", > + sizeof(virResctrlInfo), > + virResctrlInfoDispose))) The indent is off by 1 for the above 3 lines.

> + return -1; > + > + return 0; > } > > +VIR_ONCE_GLOBAL_INIT(virResctrlInfo) > > -/* > - * This function tests whether TYPE of cache control is supported or not. > - * > - * Returns 0 if not, 1 if yes and negative value on error. > - */ > -static int > -virResctrlGetCacheSupport(unsigned int level, virCacheType type) > +static virResctrlInfoPtr > +virResctrlInfoNew(void) > { > - int ret = -1; > - char *path = NULL; > - > - if (virResctrlGetCacheDir(&path, "/info", level, type) < 0) > - return -1; > + if (virResctrlInfoInitialize() < 0) > + return NULL; > > - ret = virFileExists(path); > - VIR_FREE(path); > - return ret; > + return virObjectNew(virResctrlInfoClass); > } > > > -/* > - * This function tests which TYPE of cache control is supported > - * Return values are: > - * -1: error > - * 0: none > - * 1: CAT > - * 2: CDP > - */ > +/* Info-related functions */ > int > -virResctrlGetCacheControlType(unsigned int level) > +virResctrlGetInfo(virResctrlInfoPtr *resctrl) > { > + DIR *dirp = NULL; > + char *info_path = NULL; > + char *endptr = NULL; > + char *tmp_str = NULL; > + int ret = 0; > int rv = -1; > - > - rv = virResctrlGetCacheSupport(level, VIR_CACHE_TYPE_BOTH); > - if (rv < 0) > + int type = 0; > + struct dirent *ent = NULL; > + unsigned int level = 0; > + virResctrlInfoPerLevelPtr i_level = NULL; > + virResctrlInfoPerTypePtr i_type = NULL; > + virResctrlInfoPtr tmp_info = virResctrlInfoNew(); > + > + if (!tmp_info) > return -1; > - if (rv) > - return 1; > > - rv = virResctrlGetCacheSupport(level, VIR_CACHE_TYPE_CODE); > - if (rv < 0) > - return -1; > - if (rv) > - return 2; > + rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); > + if (rv <= 0) { > + ret = rv; > + goto cleanup; > + } > > - return 0; > + while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { > + if (ent->d_type != DT_DIR) > + continue; > + > + if (ent->d_name[0] != 'L') > + continue; > + > + if (virStrToLong_uip(ent->d_name + 1, &endptr, 10, &level) < 0) > + goto cleanup; > + > + type = virResctrlTypeFromString(endptr); > + if (type < 0) > + goto cleanup; > + > + if (VIR_ALLOC(i_type) < 0) > + goto cleanup; > + > + i_type->control.scope = type; > + > + if (virFileReadValueUint(&i_type->control.max_allocation, > + SYSFS_RESCTRL_PATH "/info/%s/num_closids", > + ent->d_name) < 0) > + goto cleanup; > + > + if (virFileReadValueString(&i_type->cbm_mask, > + SYSFS_RESCTRL_PATH > + "/info/%s/cbm_mask", > + ent->d_name) < 0) > + goto cleanup; > + > + if (virFileReadValueUint(&i_type->min_cbm_bits, > + SYSFS_RESCTRL_PATH "/info/%s/min_cbm_bits", > + ent->d_name) < 0) > + goto cleanup; > + > + virStringTrimOptionalNewline(i_type->cbm_mask); > + > + if (tmp_info->nlevels <= level && > + VIR_EXPAND_N(tmp_info->levels, tmp_info->nlevels, > + level - tmp_info->nlevels + 1) < 0) > + goto cleanup;

> + > + if (!tmp_info->levels[level] && > + (VIR_ALLOC(tmp_info->levels[level]) < 0 || > + VIR_ALLOC_N(tmp_info->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0)) > + goto cleanup; > + i_level = tmp_info->levels[level]; > + > + if (i_level->types[type]) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Duplicate cache type in resctrlfs for level %u"), > + level); > + goto cleanup; > + } > + > + if (VIR_ALLOC(i_level->types[type]) < 0) > + goto cleanup; > + > + for (tmp_str = i_type->cbm_mask; *tmp_str != '\0'; tmp_str++) { > + if (!c_isxdigit(*tmp_str)) > + goto cleanup; > + > + i_type->bits += count_one_bits(virHexToBin(*tmp_str)); > + } > + > + i_level->types[type] = i_type; > + i_type = NULL; > + } > + > + *resctrl = tmp_info; > + tmp_info = NULL; Is it "by design" to have a number of failures just return 0? My read of the previous code is that it would return some sort of failure if the "dynamic" reading failed. This while loop would ignore failures and "act as if" there was no cache without letting the consumer know. Is that the intent? If so, then should the code also clear the Last error message? Probably should call that out either in the commit message or the beginning of the while loop that if we have a problem, then we're going to "quietly" continue on as if there was no cache. I'm not opposed to continuing on; however, it is different (albeit difficult to see with the diffs enabled - so I've been going back and forth between old/new code).

> + cleanup: > + VIR_DIR_CLOSE(dirp); > + VIR_FREE(info_path); > + virObjectUnref(tmp_info); > + VIR_FREE(i_type); > + return ret; > +} > + Two blank lines between functions OK so I've: Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; But obviously there are still questions, still if the design is as I've noted, then carry on... Otherwise, handle/return the error I guess which isn't rocket science.

John > +int > +virResctrlInfoGetCache(virResctrlInfoPtr resctrl, > + unsigned int level, > + unsigned long long size, > + size_t *ncontrols, > + virResctrlInfoPerCachePtr **controls) > +{ > + virResctrlInfoPerLevelPtr i_level = NULL; > + virResctrlInfoPerTypePtr i_type = NULL; > + size_t i = 0; > + int ret = -1; > + > + if (!resctrl) > + return 0; > + > + if (level >= resctrl->nlevels) > + return 0; > + > + i_level = resctrl->levels[level]; > + if (!i_level) > + return 0; > + > + for (i = 0; i < VIR_CACHE_TYPE_LAST; i++) { > + i_type = i_level->types[i]; > + if (!i_type) > + continue; > + > + /* Let's take the opportunity to update our internal information about > + * the cache size */ > + if (!i_type->size) { > + i_type->size = size; > + i_type->control.granularity = size / i_type->bits; > + if (i_type->min_cbm_bits != 1) > + i_type->control.min = i_type->min_cbm_bits * i_type->control.granularity; > + } else { > + if (i_type->size != size) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Forbidden inconsistency for resctrlfs, " > + "level %u caches have different sizes"), > + level); > + goto error; > + } > + i_type->max_cache_id++; > + } > + > + if (VIR_EXPAND_N(*controls, *ncontrols, 1) < 0) > + goto error; > + if (VIR_ALLOC((*controls)[*ncontrols - 1]) < 0) > + goto error; > + > + memcpy((*controls)[*ncontrols - 1], &i_type->control, sizeof(i_type->control)); > + } > + > + ret = 0; > + cleanup: > + return ret; > + error: > + while (*ncontrols) > + VIR_FREE((*controls)[--*ncontrols]); > + VIR_FREE(*controls); > + goto cleanup; > } [...]