On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> Introduce function for reporting CMT capability through going through
> files under /sys/fs/info/L3_MON.
> This patch is co-work with later patches and report these
> information to domain.
Do you mean you're setting the basis for future patches to provide the
capability data for monitor info?
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> ---
> src/conf/capabilities.c | 6 ++-
> src/conf/capabilities.h | 1 +
> src/util/virresctrl.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/util/virresctrl.h | 17 ++++++-
> 4 files changed, 137 insertions(+), 7 deletions(-)
>
Caveat - I didn't go back and read all the previous history on this.
Sorry there's just too much. I hope that mkletzan will also take a look
at the series since he was involved previously.
There's two things going on in this patch:
1. The actual fetch of the data into resctrl structures
2. The movement/copy of some of that data into @bank
Splitting the patches such that item 1 is separate and then item 2 is
combined with patches 3 and 4 along with some doc adjustments to
describe the output.
Of course I just peeked looking for "cache" and "bank" in docs/*.in and
found nothing /-|... Looks like docs/formatcaps.html.in needs some love
to describe <cache> and <memory_bandwidth> (how come this stuff comes to
me afterwards...)
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 326bd15..5280348 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1626,6 +1626,9 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> virBitmapFree(ptr->cpus);
> for (i = 0; i < ptr->ncontrols; i++)
> VIR_FREE(ptr->controls[i]);
> + if (ptr->monitor && ptr->monitor->features)
> + virStringListFree(ptr->monitor->features);
> + VIR_FREE(ptr->monitor);
> VIR_FREE(ptr->controls);
> VIR_FREE(ptr);
> }
> @@ -1801,7 +1804,8 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> bank->level,
> bank->size,
> &bank->ncontrols,
> - &bank->controls) < 0)
> + &bank->controls,
> + &bank->monitor) < 0)
I wonder if perhaps if it'd be better to have virResctrlInfoGetCache
just take @bank as a parameter instead of continually adding more... I'm
also not convinced @bank is the best place considering it's the same
data that repeated gets fetched/stored.
> goto cleanup;
>
> if (VIR_APPEND_ELEMENT(caps->host.caches,
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 046e275..3ed2523 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -149,6 +149,7 @@ struct _virCapsHostCacheBank {
> virBitmapPtr cpus; /* All CPUs that share this bank */
> size_t ncontrols;
> virResctrlInfoPerCachePtr *controls;
> + virResctrlInfoMonPtr monitor;
This structure notes usage for specific @level; however, from how I read
the path to the data, the data is only provided in L3. Since on output
<bank> can specify a specific level, I assume that L1 or L2 would be
possible for it; however, given how the code is written wouldn't that
mean @monitor data is included as well?
Additionally from how I read things it seems the same data is repeated
for each bank id='#' found. So is the data unique to a bank by id or is
unique to the level regardless of the bank? If the former, then the
data needs to be properly split so it can be shown to be different for
each id. If the latter, then <monitor> wouldn't seem to need to be a
child of <bank>. It's not clear whether it's then a child or peer of
<cache>.
> };
>
> typedef struct _virCapsHostMemBWNode virCapsHostMemBWNode;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 4b5442f..2f6923a 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -146,6 +146,8 @@ struct _virResctrlInfo {
> size_t nlevels;
>
> virResctrlInfoMemBWPtr membw_info;
> +
> + virResctrlInfoMonPtr monitor_info;
> };
>
>
> @@ -171,6 +173,9 @@ virResctrlInfoDispose(void *obj)
> VIR_FREE(level);
> }
>
> + if (resctrl->monitor_info)
> + virStringListFree(resctrl->monitor_info->features);
> + VIR_FREE(resctrl->monitor_info);
> VIR_FREE(resctrl->membw_info);
> VIR_FREE(resctrl->levels);
> }
> @@ -556,6 +561,81 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
>
>
Could add a few comments here to describe what's being provided here and
how it fits (regardless of where in the schema of things it ends up).
> static int
> +virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
> +{
> + int rv = -1;
> + char *featurestr = NULL;
> + char **lines = NULL> + size_t nlines = 0;
> + size_t i = 0;
> + int ret = -1;
> + virResctrlInfoMonPtr info = NULL;
> +
> + if (VIR_ALLOC(info) < 0)
> + return -1;
> +
> + rv = virFileReadValueUint(&info->max_allocation,
> + SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
> + if (rv == -2) {
> + /* The file doesn't exist, so it's unusable for us,
> + * probably resource monitoring feature unsupported */
> + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' "
> + "does not exist");
> +
> + ret = 0;
> + goto cleanup;
> + } else if (rv < 0) {
> + /* Other failures are fatal, so just quit */
> + goto cleanup;
> + }
> +
> + rv = virFileReadValueUint(&info->cache_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"));
> + }
> + 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;
> +
> + lines = virStringSplitCount(featurestr, "\n", 0, &nlines);
> +
> + for (i = 0; i < nlines; i++) {
> + if (STREQLEN(lines[i], "llc_", strlen("llc_")) ||
> + STREQLEN(lines[i], "mbm_", strlen("mbm_"))) {
Consider using STRPREFIX.
> + if (virStringListAdd(&info->features, lines[i]) < 0)
> + goto cleanup;
> + info->nfeatures++;
> + }
So we get a list filtered by prefixes "llc_" and "mbm_"
Eventually this list gets pared down again to just "llc_". None of the
subsequent patches do anything with "mbm_" other than list it in
capabilities XML output.
Sure "mbm_" could be used in the future, but the question that comes to
mind is why are initially filtering at all? That is, why not just
replace lines/nlines with info->features and info->nfeatures? That then
provides "everything" supported in the tree, right?
I would figure this code would be just mirroring what's available. It's
then up to the upper layers or other code to decide what to do with the
list.
> + }
> +
> + VIR_FREE(featurestr);
> + virStringListFree(lines);
> + resctrl->monitor_info = info;
> + return 0;
consider using cleanup as part of the processing and thus removing the
need to have multiple VIR_FREE(featurestr) and virStringListFree(lines).
If you add a char **features = NULL; which you use to perform the
virStringListAdd calls and then
VIR_STEAL_PTR(info->features, features);
VIR_STEAL_PTR(resctrl->monitor_info, info);
ret = 0;
and fall through allowing the featurestr, lines, and features to be used
below.
Of course that all assumes it's really necessary to do the filtering.
There's also the VIR_AUTOPTR stuff added which I'm not exactly sure how
to use yet as I wasn't part of that effort.
> +
> + cleanup:
> + VIR_FREE(featurestr);
> + virStringListFree(lines);
> + virStringListFree(info->features);
> + VIR_FREE(info);
> + return ret;
> +}
> +
> +
> +static int
> virResctrlGetInfo(virResctrlInfoPtr resctrl)
> {
> DIR *dirp = NULL;
> @@ -569,6 +649,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
> if (ret < 0)
> goto cleanup;
>
> + ret = virResctrlGetMonitorInfo(resctrl);
> + if (ret < 0)
> + goto cleanup;
> +
> ret = virResctrlGetCacheInfo(resctrl, dirp);
> if (ret < 0)
> goto cleanup;
> @@ -654,16 +738,21 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
> unsigned int level,
> unsigned long long size,
> size_t *ncontrols,
> - virResctrlInfoPerCachePtr **controls)
> + virResctrlInfoPerCachePtr **controls,
> + virResctrlInfoMonPtr *monitor)
> {
> virResctrlInfoPerLevelPtr i_level = NULL;
> virResctrlInfoPerTypePtr i_type = NULL;
> + virResctrlInfoMonPtr cachemon = NULL;
> size_t i = 0;
> int ret = -1;
>
> if (virResctrlInfoIsEmpty(resctrl))
> return 0;
>
> + if (VIR_ALLOC(cachemon) < 0)
> + return -1;
> +
> /* Let's take the opportunity to update the number of last level
> * cache. This number of memory bandwidth controller is same with
> * last level cache */
> @@ -716,14 +805,35 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
> memcpy((*controls)[*ncontrols - 1], &i_type->control, sizeof(i_type->control));
> }
>
> - ret = 0;
> - cleanup:
> - return ret;
> + cachemon->max_allocation = 0;
> +
> + if (resctrl->monitor_info) {
> + virResctrlInfoMonPtr info = resctrl->monitor_info;
> +
> + cachemon->max_allocation = info->max_allocation;
> + cachemon->cache_threshold = info->cache_threshold;
> + for (i = 0; i < info->nfeatures; i++) {
> + /* Only cares about last level cache */
> + if (STREQLEN(info->features[i], "llc_", strlen("llc_"))) {
Again use STRPREFIX instead.
> + if (virStringListAdd(&cachemon->features,
> + info->features[i]) < 0)
> + goto error;
> + cachemon->nfeatures++;
> + }
> + }
This code further filters our "llc_" and "mbm_" list into just "llc_".
Not sure I understand why we only "care about last level cache" values
just yet. Of course I see that is all that gets added the subsequent
patch capabilities output, but is that really what's wanted. Are we
going to start seeing patches that start expanding this list? Why
limit/filter now?
> + }
> +
> + if (cachemon->features)
> + *monitor = cachemon;
if (!cachemon->features), then @cachemon is leaked, consider using:
VIR_STEAL_PTR(*monitor, cachemon);
in the if condition, then
VIR_FREE(cachemon);
or just the VIR_FREE(cachemon); as an else. IDC either way. Of course,
it's still not quite clear what's going on.
Perhaps, you should have an API that gets all the names of the values
prefixed by some string, IOW:
virResctrlInfoGetMonitorPrefix(resctrl, filter)
where filter is a "const char *filter"
it would return that cachemon list whether it's NULL, empty, or full
anything. Let the caller decide what to do with it.
I haven't looked beyond the first 4 patches, so how things are used
later on may determine what API's you could need. The relationship to
<cache> and <bank> isn't clear.
> +
> + return 0;
> error:
> while (*ncontrols)
> VIR_FREE((*controls)[--*ncontrols]);
> VIR_FREE(*controls);
> - goto cleanup;
> + virStringListFree(cachemon->features);
> + VIR_FREE(cachemon);
> + return ret;
> }
>
>
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index cfd56dd..51bb68b 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -61,6 +61,19 @@ struct _virResctrlInfoMemBWPerNode {
> unsigned int max_allocation;
> };
>
> +typedef struct _virResctrlInfoMon virResctrlInfoMon;
> +typedef virResctrlInfoMon *virResctrlInfoMonPtr;
> +/* Information about resource monitoring group */
> +struct _virResctrlInfoMon {
> + /* null-terminal string list for hw supported monitor feature */
> + char **features;
> + size_t nfeatures;
> + /* Maximum number of simultaneous allocations */
> + unsigned int max_allocation;
What kind of allocations? From your cover you state maximum number of
monitoring groups that could be created, but it's impossible to know how
this value is expected to be used by what's provided as a comment here.
The code shows this is the value from /info/L3_MON/num_rmids - I don't
see the correlation in FS name to structure name. Since you later print
a unit of 'B' I assume it's bytes of something, but the comment seems to
imply a maximum simultaneous number of something.
Perhaps if documentation was added I would have had my answer without
needing to go research that is.
> + /* determines the occupancy at which an RMID can be freed */
Again, alone this comment is difficult to decipher as it relates to the
structure field name. The code shows the value read is
"/info/L3_MON/max_threshold_occupancy". It's not clear what "occupancy"
means. Is there something related to this number that some consumer
could change that would improve some performance?
John
> + unsigned int cache_threshold;
> +};
> +
> typedef struct _virResctrlInfo virResctrlInfo;
> typedef virResctrlInfo *virResctrlInfoPtr;
>
> @@ -72,7 +85,9 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
> unsigned int level,
> unsigned long long size,
> size_t *ncontrols,
> - virResctrlInfoPerCachePtr **controls);
> + virResctrlInfoPerCachePtr **controls,
> + virResctrlInfoMonPtr *monitor);
> +
>
> int
> virResctrlInfoGetMemoryBandwidth(virResctrlInfoPtr resctrl,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
