[libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
Wang, Huaqiang
huaqiang.wang at intel.com
Thu Oct 11 08:51:57 UTC 2018
On 10/11/2018 3:13 AM, John Ferlan wrote:
>
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>> Add interfaces monitor group to support operations such
>> as add PID, set ID, remove group ... etc.
>>
>> The interface for getting cache occupancy information
>> from the monitor is also added.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>> ---
>> src/libvirt_private.syms | 6 ++
>> src/util/virresctrl.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++-
>> src/util/virresctrl.h | 23 ++++++
>> 3 files changed, 236 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index a878083..c93d19f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2683,7 +2683,13 @@ virResctrlInfoNew;
>> virResctrlMonitorAddPID;
>> virResctrlMonitorCreate;
>> virResctrlMonitorDeterminePath;
>> +virResctrlMonitorGetCacheLevel;
>> +virResctrlMonitorGetCacheOccupancy;
>> +virResctrlMonitorGetID;
>> virResctrlMonitorNew;
>> +virResctrlMonitorRemove;
>> +virResctrlMonitorSetCacheLevel;
>> +virResctrlMonitorSetID;
>>
>>
>> # util/virrotatingfile.h
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index b3d20cc..fca1f6f 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -225,11 +225,19 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
>> }
>>
>>
>> +
>> +/*
>> + * virResctrlAlloc and virResctrlMonitor are representing a resource control
>> + * group (in XML under cputune/cachetune and consequently a directory under
>> + * /sys/fs/resctrl). virResctrlAlloc is the data structure for resource
>> + * allocation, while the virResctrlMonitor represents the resource monitoring
>> + * part.
>> + */
>> +
>> /* virResctrlAlloc */
>>
>> /*
>> - * virResctrlAlloc represents one allocation (in XML under cputune/cachetune and
>> - * consequently a directory under /sys/fs/resctrl). Since it can have multiple
>> + * virResctrlAlloc represents one allocation. Since it can have multiple
>
> I would think that perhaps the comments changing here would go earlier
> when virResctrlMonitor was introduced. The comment with the single
> virResctrlAlloc could be changed to "virResctrlAlloc and
> virResctrlMonitor", then merge in what you've typed above.
>
>> * parts of multiple caches allocated it is represented as bunch of nested
>> * sparse arrays (by sparse I mean array of pointers so that each might be NULL
>> * in case there is no allocation for that particular cache allocation (level,
>> @@ -347,6 +355,8 @@ struct _virResctrlMonitor {
>> /* libvirt-generated path in /sys/fs/resctrl for this particular
>> * monitor */
>> char *path;
>> + /* The cache 'level', special for cache monitor */
>> + unsigned int cache_level;
>> };
>>
>>
>> @@ -2510,6 +2520,27 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>>
>>
>> int
>> +virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
>> + const char *id)
>> +{
>> + if (!id) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Resctrl monitor 'id' cannot be NULL"));
>> + return -1;
>> + }
>> +
>> + return VIR_STRDUP(monitor->id, id);
>> +}
>> +
>> +
>> +const char *
>> +virResctrlMonitorGetID(virResctrlMonitorPtr monitor)
>> +{
>> + return monitor->id;
>> +}
>> +
>> +
>> +int
>> virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>> virResctrlMonitorPtr monitor,
>> const char *machinename)
>> @@ -2534,3 +2565,177 @@ virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>> virResctrlUnlock(lockfd);
>> return ret;
>> }
>> +
>> +
>> +int
>
> The eventual only caller never checks status...
That's true, and I noticed that. The back-end logic is copied from
virResctrlAllocRemove.
Maybe with a change of removing the following
virReportSystemError lines.
>
>> +virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
>> +{
>> + int ret = 0;
>> +
>
> So unlike Alloc, @monitor cannot be NULL...
@monitor is not allowed to be NULL. This is guaranteed by the caller.
>
>> + if (!monitor->path)
>> + return 0;
>> +
>> + VIR_DEBUG("Removing resctrl monitor%s", monitor->path);
>> + if (rmdir(monitor->path) != 0 && errno != ENOENT) {
>> + virReportSystemError(errno,
>> + _("Unable to remove %s (%d)"),
>> + monitor->path, errno);
>> + ret = -errno;
>> + VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno);
>
> Either virReportSystemError if you're going to handle the returned
> status or VIR_ERROR if you're not (and are just ignoring), but not both.
I would like to remove the virReportSystemError lines and keep the VIR_ERROR line.
>
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>> +int
>> +virResctrlMonitorSetCacheLevel(virResctrlMonitorPtr monitor,
>> + unsigned int level)
>> +{
>> + /* Only supports cache level 3 CMT */
>> + if (level != 3) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Invalid resctrl monitor cache level"));
>> + return -1;
>> + }
>> +
>> + monitor->cache_level = level;
>> +
>> + return 0;
>> +}
>> +
>> +unsigned int
>> +virResctrlMonitorGetCacheLevel(virResctrlMonitorPtr monitor)
>> +{
>> + return monitor->cache_level;
>> +}
>
> Based on usage, maybe we should just give up on API's like this. Create
> a VIR_RESCTRL_MONITOR_CACHE_LEVEL (or something like it)... Then use it
> at least for now when reading/supplying the level. Thus we leave it to
> future developer to create the API's and handle the new levels...
>
> If when we Parse we don't find the constant, then error.
Do you mean removing the 'virResctrlMonitorSetCacheLevel'
and 'virResctrlMonitorGetCacheLevel' two functions from here, and processing
the monitor cache level information directory in domain_conf.c during XML
parsing and format process.
>
>> +
>> +
>> +/*
>> + * virResctrlMonitorGetStatistic
>
> Usually just GetStats is fine or GetOneStat
I prefer GetStats -> virResctrlMonitorGetStats
>
>> + *
>> + * @monitor: The monitor that the statistic data will be retrieved from.
>> + * @resource: The name for resource name. 'llc_occpancy' for cache resource.
>
> occupancy
OK
>
>
>> + * "mbm_totol_bytes" and "mbm_local_bytes" for memory bandwidth resource.
>
> mem_total_bytes
Yes, I spell a wrong word. will be corrected.
>
>
> Although the actual names could go below when describing [1]
The describe of @resource would be trimmed:
@resource: The name for resource name.
>
>
>> + * @len: The array length for @ids, and @vals
>> + * @ids: The id array for resource statistic information, ids[0]
>> + * stores the first node id value, ids[1] stores the second node id value,
>> + * ... and so on.
>> + * @vals: The resource resource utilization information array. vals[0]
>> + * stores the cache or memory bandwidth utilization value for first node,
>> + * vals[1] stores the second value ... and so on.
>> + *
>
> [1] e.g. here - what you'd expect @resource to be...
* @vals: The data array of the current @resource utilization information.
* The first element of @vals stores the first node's data, the second
* array element stores the second node data if there is multi-node system.
* Similarly the third array element stores the third ... and so on.
* If @resource is 'llc_occupancy', the array stores the cache occupancy
* information for all nodes. If @resource is 'mbm_total_bytes' or
* 'mbm_local_bytes', then the array stores memory bandwidth related
* information for all nodes.
These comments might be changed, as we want to combine @ids with @vals.
>
>
>> + * Get cache or memory bandwidth utilization information from monitor that
>> + * specified by @id.
>> + *
>> + * Returns 0 for success, -1 for error.
>> + */
>> +static int
>> +virResctrlMonitorGetStatistic(virResctrlMonitorPtr monitor,
>> + const char *resource,
>> + size_t *len,
>> + unsigned int **ids,
>> + unsigned int **vals)
>> +{
>> + int rv = -1;
>> + int ret = -1;
>> + size_t nids = 0;
>> + size_t nvals = 0;
>> + DIR *dirp = NULL;
>> + char *datapath = NULL;
>> + struct dirent *ent = NULL;
>> +
>> + if (!monitor) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Invalid resctrl monitor"));
>> + return -1;
>> + }
>> +
>> + if (virAsprintf(&datapath, "%s/mon_data", monitor->path) < 0)
>> + return -1;
>> +
>> + if (virDirOpen(&dirp, datapath) < 0)
>> + goto cleanup;
>> +
>> + *len = 0;
>> + while (virDirRead(dirp, &ent, datapath) > 0) {
>> + char *str_id = NULL;
>> + unsigned int id = 0;
>> + unsigned int val = 0;
>> + size_t i = 0;
>> + size_t cur_id_pos = 0;
>> + unsigned int tmp_id = 0;
>> + unsigned int tmp_val = 0;
>> +
>> + /* Looking for directory that contains resource utilization
>> + * information file. The directory name is arranged in format
>> + * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and
>> + * "mon_l3_01" are two target directories for a two nodes system
>> + * with resource utilization data file for each node respectively.
>> + */
>> + if (ent->d_type != DT_DIR)
>> + continue;
>> +
>> + if (STRNEQLEN(ent->d_name, "mon_L", 5))
>> + continue;
>> +
>> + str_id = strchr(ent->d_name, '_');
>> + if (!str_id)
>> + continue;
> > +
>> + str_id = strchr(++str_id, '_');
>> + if (!str_id)
>> + continue;
>
> I think all of the above could be replaced by:
>
> if (!(str_id = STRSKIP(ent->d_name, "mon_L")))
> continue;
>
> I personally am not a fan of pre-auto-incr. I get particularly
> uncomfortable when it involves pointer arithmetic... But I think it's
> unnecessary if you use STRSKIP
The interesting target directory name is like 'MON_L3_00' or 'MON_L3CODE_00'
if CDP is enabled. The last two numbers after second '_' character is the ID
number, now we need to locate the second '_' character.
One STRSKIP is not enough, still need a call of strchr to locate the second '_' in
following way:
if (!(str_id = STRSKIP(ent->d_name, "mon_L")))
continue;
str_id = strchr(str_id , '_');
if (!str_id)
continue;
>
>
>> +
>> + if (virStrToLong_uip(++str_id, NULL, 0, &id) < 0)
>> + goto cleanup;
>> +
>> + rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
>> + ent->d_name, resource);
>> + if (rv == -2) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("File '%s/%s/%s' does not exist."),
>> + datapath, ent->d_name, resource);
>> + }
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + if (VIR_APPEND_ELEMENT(*ids, nids, id) < 0)
>> + goto cleanup;
>> +
>> + if (VIR_APPEND_ELEMENT(*vals, nvals, val) < 0)
>> + goto cleanup;
>
> If you had a single structure for id and val, then...
How about:
struct _virResctrlMonitorStats {
unsigned int id;
unsigned int val;
};
>
>
>> +
>> + /* Sort @ids and @vals arrays in the ascending order of id */
>> + cur_id_pos = nids - 1;
>> + for (i = 0; i < cur_id_pos; i++) {
>> + if ((*ids)[cur_id_pos] < (*ids)[i]) {
>> + tmp_id = (*ids)[cur_id_pos];
>> + tmp_val = (*vals)[cur_id_pos];
>> + (*ids)[cur_id_pos] = (*ids)[i];
>> + (*vals)[cur_id_pos] = (*vals)[i];
>> + (*ids)[i] = tmp_id;
>> + (*vals)[i] = tmp_val;
>> + }
>
> ...qsort would be a much better option than the above open code...
If @ids and @vals are combined in one structure, then is very convenient to use
'qsort' to sort a structure.
The sorting action will be moved out of the loop, before a successful return.
Will be done.
>
>
>> + }
>> + }
>> +
>> + *len = nids;
>> + ret = 0;
>> + cleanup:
>> + VIR_FREE(datapath);
>> + VIR_DIR_CLOSE(dirp);
>> + return ret;
>> +}
>> +
>> +
>> +/* Get cache occupancy data from @monitor */
>> +int
>> +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
>> + size_t *nbank,
>> + unsigned int **bankids,
>> + unsigned int **bankcaches)
>> +{
>> + return virResctrlMonitorGetStatistic(monitor, "llc_occupancy",
>> + nbank, bankids, bankcaches);
>> +}
>
> I think the above two may be a case for waiting until they're needed,
> but I haven't got that far yet. IOW: Some extraction and reordering.
This function is used in patch 18, maybe move these two functions into patch18.
Will be done.
>
>
> John
Thanks for review.
Huaqiang
>
>> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>> index 1efe394..6137fee 100644
>> --- a/src/util/virresctrl.h
>> +++ b/src/util/virresctrl.h
>> @@ -202,7 +202,30 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>> const char *machinename);
>>
>> int
>> +virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
>> + const char *id);
>> +
>> +const char *
>> +virResctrlMonitorGetID(virResctrlMonitorPtr monitor);
>> +
>> +int
>> virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>> virResctrlMonitorPtr monitor,
>> const char *machinename);
>> +
>> +int
>> +virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
>> +
>> +int
>> +virResctrlMonitorSetCacheLevel(virResctrlMonitorPtr monitor,
>> + unsigned int level);
>> +
>> +unsigned int
>> +virResctrlMonitorGetCacheLevel(virResctrlMonitorPtr monitor);
>> +
>> +int
>> +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
>> + size_t *nbank,
>> + unsigned int **bankids,
>> + unsigned int **bankcaches);
>> #endif /* __VIR_RESCTRL_H__ */
>>
More information about the libvir-list
mailing list