[libvirt] [PATCH 12/21] resctrl: Instantiate all resctrl information at once
Martin Kletzander
mkletzan at redhat.com
Wed Nov 15 12:03:35 UTC 2017
On Tue, Nov 14, 2017 at 10:57:26AM -0500, John Ferlan wrote:
>
>
>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 at redhat.com>
>> ---
>> 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"...
>
You're right, this should be:
virResctrlInfoPerLevelPtr level = resctrl->levels[i];
It's just leftover from previous version that had:
while (resctrl->nlevels)
>> +
>> + if (!level)
>> + continue;
>>
>> + if (level->types) {
>> + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++)
>
>level->types[j]->cbm_mask is leaked.
>
Fixed
>> + 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.
>
Good catch, this used to be called virResctrlAllocClass in one of the
past versions before "automatic" rename :D Hence the off-by-one indent ;)
>> + 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;
Also one more indentation is wrong here. Found by selecting the whole
file and letting it auto indent, this was the only missing change, so it
should be fine as well.
Fixed now.
>> +
>> + 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).
>
Some particular ones should just be "resctrl is not supported here,
don't fail starting the daemon because of that". However those are
handled, this one should have `int ret = -1;` in the top and `ret = 0;`
before the `cleanup:` label of course. Thanks for seeing that.
This used to return the pointer to the info previously, that's where the
weirdness comes from.
>> + 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 at redhat.com>
>
>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.
>
I added virReportError for every situation that did not report the error
and silently went for goto cleanup; Also changed the ret value as
described above.
>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;
>> }
>
>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171115/95c5cd32/attachment-0001.sig>
More information about the libvir-list
mailing list