[libvirt] [PATCH 2/9] util: Add virResctrlInfo
Martin Kletzander
mkletzan at redhat.com
Mon Jan 22 14:23:46 UTC 2018
On Tue, Jan 02, 2018 at 03:08:30PM -0500, John Ferlan wrote:
>
>
>On 12/13/2017 10:39 AM, Martin Kletzander wrote:
>> This will make the current functions obsolete and it will provide more
>> information to the virresctrl module so that it can be used later.
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>> po/POTFILES.in | 1 +
>> src/libvirt_private.syms | 3 +
>> src/util/virresctrl.c | 301 +++++++++++++++++++++++++++++++++++++++++++++++
>> src/util/virresctrl.h | 19 +++
>> 4 files changed, 324 insertions(+)
>>
>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>> index c1fa23427eff..8382ee633621 100644
>> --- a/po/POTFILES.in
>> +++ b/po/POTFILES.in
>> @@ -252,6 +252,7 @@ src/util/virportallocator.c
>> src/util/virprocess.c
>> src/util/virqemu.c
>> src/util/virrandom.c
>> +src/util/virresctrl.c
>> src/util/virrotatingfile.c
>> src/util/virscsi.c
>> src/util/virscsihost.c
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index de4ec4d442c9..75be612a2f13 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2550,6 +2550,9 @@ virCacheTypeFromString;
>> virCacheTypeToString;
>> virResctrlGetCacheControlType;
>> virResctrlGetCacheInfo;
>> +virResctrlGetInfo;
>> +virResctrlInfoGetCache;
>> +virResctrlInfoNew;
>>
>>
>> # util/virrotatingfile.h
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index 050a08178e24..6fd1ceb587cf 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -25,12 +25,15 @@
>> #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,6 +58,304 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST,
>> "CODE",
>> "DATA")
>>
>> +
>> +/* 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;
>> +
>> + /* Our computed information from the above */
>> + unsigned int bits;
>> + unsigned int max_cache_id;
>> +
>> + /* 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;
>> +
>> + /* Information that we will return upon request (this is public struct) as
>> + * until now all the above is internal to this module */
>> + virResctrlInfoPerCache control;
>> +};
>> +
>> +typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
>> +typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
>> +struct _virResctrlInfoPerLevel {
>> + virResctrlInfoPerTypePtr *types;
>> +};
>> +
>> +struct _virResctrlInfo {
>> + virObject parent;
>> +
>> + virResctrlInfoPerLevelPtr *levels;
>> + size_t nlevels;
>> +};
>> +
>> +static virClassPtr virResctrlInfoClass;
>> +
>> +static void
>> +virResctrlInfoDispose(void *obj)
>> +{
>> + size_t i = 0;
>> + size_t j = 0;
>> +
>> + virResctrlInfoPtr resctrl = obj;
>> +
>> + for (i = 0; i < resctrl->nlevels; i++) {
>> + virResctrlInfoPerLevelPtr level = resctrl->levels[i];
>> +
>> + if (!level)
>> + continue;
>> +
>> + if (level->types) {
>> + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> + if (level->types[j])
>> + VIR_FREE(level->types[j]->cbm_mask);
>> + VIR_FREE(level->types[j]);
>> + }
>> + }
>> + VIR_FREE(level->types);
>> + VIR_FREE(level);
>> + }
>> +
>> + VIR_FREE(resctrl->levels);
>> +}
>> +
>> +
>> +static int virResctrlInfoOnceInit(void)
>
>static int
>virRes....
>
>> +{
>> + if (!(virResctrlInfoClass = virClassNew(virClassForObject(),
>> + "virResctrlInfo",
>> + sizeof(virResctrlInfo),
>> + virResctrlInfoDispose)))
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +
>> +VIR_ONCE_GLOBAL_INIT(virResctrlInfo)
>> +
>> +
>> +virResctrlInfoPtr
>> +virResctrlInfoNew(void)
>> +{
>> + if (virResctrlInfoInitialize() < 0)
>> + return NULL;
>> +
>> + return virObjectNew(virResctrlInfoClass);
>> +}
>> +
>> +
>> +/* Info-related functions */
>> +bool
>> +virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
>> +{
>> + size_t i = 0;
>> + size_t j = 0;
>> +
>> + if (!resctrl)
>> + return true;
>> +
>> + for (i = 0; i < resctrl->nlevels; i++) {
>> + virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
>> +
>> + if (!i_level)
>> + continue;
>> +
>> + for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
>> + if (i_level->types[j])
>> + return false;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>
>Two blank lines
>
>> +int
>> +virResctrlGetInfo(virResctrlInfoPtr resctrl)
>> +{
>> + DIR *dirp = NULL;
>> + char *info_path = NULL;
>> + char *endptr = NULL;
>> + char *tmp_str = NULL;
>> + int ret = -1;
>> + int rv = -1;
>> + int type = 0;
>> + struct dirent *ent = NULL;
>> + unsigned int level = 0;
>> + virResctrlInfoPerLevelPtr i_level = NULL;
>> + virResctrlInfoPerTypePtr i_type = NULL;
>> +
>> + rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
>> + if (rv <= 0) {
>> + ret = rv;
>> + goto cleanup;
>> + }
>> +
>> + 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) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot parse resctrl cache info level"));
>> + goto cleanup;
>> + }
>> +
>> + type = virResctrlTypeFromString(endptr);
>> + if (type < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot parse resctrl cache info type"));
>> + goto cleanup;
>> + }
>> +
>> + if (VIR_ALLOC(i_type) < 0)
>> + goto cleanup;
>> +
>> + i_type->control.scope = type;
>> +
>> + rv = virFileReadValueUint(&i_type->control.max_allocation,
>> + SYSFS_RESCTRL_PATH "/info/%s/num_closids",
>> + ent->d_name);
>> + if (rv == -2)
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot get num_closids from resctrl cache info"));
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + rv = virFileReadValueString(&i_type->cbm_mask,
>> + SYSFS_RESCTRL_PATH
>> + "/info/%s/cbm_mask",
>> + ent->d_name);
>> + if (rv == -2)
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot get cbm_mask from resctrl cache info"));
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + rv = virFileReadValueUint(&i_type->min_cbm_bits,
>> + SYSFS_RESCTRL_PATH "/info/%s/min_cbm_bits",
>> + ent->d_name);
>> + if (rv == -2)
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot get min_cbm_bits from resctrl cache info"));
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + virStringTrimOptionalNewline(i_type->cbm_mask);
>
>Move this up a few lines to after cbm_mask successfully read...
>
>> +
>> + if (resctrl->nlevels <= level &&
>> + VIR_EXPAND_N(resctrl->levels, resctrl->nlevels,
>> + level - resctrl->nlevels + 1) < 0)
>> + goto cleanup;
>> +
>> + if (!resctrl->levels[level] &&
>> + (VIR_ALLOC(resctrl->levels[level]) < 0 ||
>> + VIR_ALLOC_N(resctrl->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0))
>> + goto cleanup;
>> + i_level = resctrl->levels[level];
>> +
>> + if (i_level->types[type]) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Duplicate cache type in resctrlfs for level %u"),
>> + level);
>> + goto cleanup;
>> + }
>> +
>> + for (tmp_str = i_type->cbm_mask; *tmp_str != '\0'; tmp_str++) {
>> + if (!c_isxdigit(*tmp_str)) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot parse cbm_mask from resctrl cache info"));
>> + goto cleanup;
>> + }
>> +
>> + i_type->bits += count_one_bits(virHexToBin(*tmp_str));
>> + }
>> +
>> + VIR_STEAL_PTR(i_level->types[type], i_type);
>> + }
>> +
>> + ret = 0;
>> + cleanup:
>> + VIR_DIR_CLOSE(dirp);
>> + VIR_FREE(info_path);
>> + VIR_FREE(i_type);
>
>In a cleanup/failure path i_type.cbm_mask is leaked, thus before this
>
>if (i_type)
> VIR_FREE(i_type->cbm_mask);
>
>or go with the VIR_STEAL_PTR type logic for a local @cbm_mask to be
>stored in i_type->cbm_mask (IDC whichever way you prefer)
>
>> + return ret;
>> +}
>> +
>> +
>> +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 (virResctrlInfoIsEmpty(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) {
>
>Not really a boolean or pointer comparison...
>
>> + 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 %u cache size %llu not match expected %llu
>levek, i_type->size, size
>
>> + 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:
>
>Is this really necessary? The caller on failure will call
>virCapsHostCacheBankFree(bank) which does the free's.
>
It's not, but it's good coding practice not leaving anything (that you were
changing/allocating) in an inconsistent state when exiting a function.
Fixed everything else.
-------------- 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/20180122/37d2d7ac/attachment-0001.sig>
More information about the libvir-list
mailing list