Re: [libvirt] [PATCH 06/10] util: Introduce resctrl monitor for CMT

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 5, 2018 11:00 PM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; Subject: Re: [libvirt] [PATCH 06/10] util: Introduce resctrl monitor for CMT On 08/27/2018 07:23 AM, Wang Huaqiang wrote: > 'virResctrlAllocMon' denotes a resctrl monitor reporting the resource > consumption information. > > This patch introduced the interfaces for resctrl monitor. > > Relationship of 'resctrl allocation' and 'resctrl monitor': > 1. resctrl monitor monitors resources (cache or memory bandwidth) of > particular allocation. > 2. resctrl allocation may refer to the 'default' allocation if no > dedicated resource 'control' applied to it. The 'default' allocation > enjoys remaining resource that not allocated. > 3. resctrl monitor belongs to 'default' allocation if no 'cachetune' > specified in XML file. > 4. one resctrl allocation may have several monitors. It is also > permitted that there is no resctrl monitor associated with an > allocation. > > Key data structures: > > + struct _virResctrlAllocMon { > + char *id; > + char *path; > + }; > > struct _virResctrlAlloc { > virObject parent; > > @@ -276,6 +289,12 @@ struct _virResctrlAlloc { > virResctrlAllocMemBWPtr mem_bw; > + virResctrlAllocMonPtr *monitors; > + size_t nmonitors; > } > > Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; > --- > src/libvirt_private.syms | 6 + > src/util/virresctrl.c | 361 ++++++++++++++++++++++++++++++++++++++++++++++- > src/util/virresctrl.h | 31 ++++ > 3 files changed, 394 insertions(+), 4 deletions(-) > Similar to the previous patch - there's a bit too much going on for just one patch here. Introducing "default_group" and "monitors". This needs some separation.

I am not going to look at this patch. I think you really need to describe this default_group concept quite a bit more as you'd be totally changing the meaning of alloc->path by "removing" everything after the SYSFS_RESCTRL_PATH. What's the purpose of alloc->path then in this environment?

John > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index > 47ea35f..1439327 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2645,12 +2645,17 @@ virCacheKernelTypeFromString; > virCacheKernelTypeToString; virCacheTypeFromString; > virCacheTypeToString; > +virResctrlAllocAddMonitorPID; > virResctrlAllocAddPID; > virResctrlAllocCreate; > +virResctrlAllocCreateMonitor; > +virResctrlAllocDeleteMonitor; > +virResctrlAllocDetermineMonitorPath; > virResctrlAllocDeterminePath; > virResctrlAllocForeachCache; > virResctrlAllocForeachMemory; > virResctrlAllocFormat; > +virResctrlAllocGetCacheOccupancy; > virResctrlAllocGetID; > virResctrlAllocGetUnused; > virResctrlAllocNew; > @@ -2658,6 +2663,7 @@ virResctrlAllocRemove; > virResctrlAllocSetCacheSize; virResctrlAllocSetID; > virResctrlAllocSetMemoryBandwidth; > +virResctrlAllocSetMonitor; > virResctrlInfoGetCache; > virResctrlInfoNew; > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index > b3bae6e..7215a47 100644 > --- a/src/util/virresctrl.c > +++ b/src/util/virresctrl.c > @@ -257,6 +257,19 @@ struct _virResctrlAllocMemBW { > size_t nbandwidths; > }; > > + > +typedef struct _virResctrlAllocMon virResctrlAllocMon; typedef > +virResctrlAllocMon *virResctrlAllocMonPtr; > +/* virResctrlAllocMon denotes a resctrl monitoring group reporting > +the resource > + * consumption information for resource of either cache or memory > + * bandwidth. */ > +struct _virResctrlAllocMon { > + /* monitoring group identifier, should be unique in scope of allocation */ > + char *id; > + /* directory path under /sys/fs/resctrl*/ > + char *path; > +}; > + > struct _virResctrlAlloc { > virObject parent; > > @@ -265,11 +278,21 @@ struct _virResctrlAlloc { > > virResctrlAllocMemBWPtr mem_bw; > > + /* monintoring groups associated with current resource allocation > + * it might report resource consumption information at a finer > + * granularity */ > + virResctrlAllocMonPtr *monitors; > + size_t nmonitors; > + > /* The identifier (any unique string for now) */ > char *id; > /* libvirt-generated path in /sys/fs/resctrl for this particular > * allocation */ > char *path; > + /* is this a default resctrl group? > + * true : default group, directory path equals '/sys/fs/resctrl' > + * false: non-default group */ > + bool default_group; > }; > > > @@ -315,6 +338,13 @@ virResctrlAllocDispose(void *obj) > VIR_FREE(alloc->mem_bw); > } > > + for (i = 0; i < alloc->nmonitors; i++) { > + virResctrlAllocMonPtr monitor = alloc->monitors[i]; > + VIR_FREE(monitor->id); > + VIR_FREE(monitor->path); > + VIR_FREE(monitor); > + } > + VIR_FREE(alloc->monitors); > VIR_FREE(alloc->id); > VIR_FREE(alloc->path); > VIR_FREE(alloc->levels); > @@ -805,6 +835,7 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, > memcpy((*controls)[*ncontrols - 1], &i_type->control, sizeof(i_type- >control)); > } > > + cachemon->nfeatures = 0; > cachemon->max_allocation = 0; > > if (resctrl->monitor_info) { > @@ -817,7 +848,7 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, > if (STREQLEN(info->features[i], "llc_", strlen("llc_"))) { > if (virStringListAdd(&cachemon->features, > info->features[i]) < 0) > - goto error; > + goto error; > cachemon->nfeatures++; > } > } > @@ -841,10 +872,19 @@ virResctrlInfoGetCache(virResctrlInfoPtr > resctrl, virResctrlAllocPtr > virResctrlAllocNew(void) > { > + virResctrlAllocPtr ret = NULL; > + > if (virResctrlInitialize() < 0) > return NULL; > > - return virObjectNew(virResctrlAllocClass); > + ret = virObjectNew(virResctrlAllocClass); > + if (!ret) > + return NULL; > + > + /* By default, a resource group is a default group */ > + ret->default_group = true; > + > + return ret; > } > > > @@ -861,6 +901,9 @@ virResctrlAllocIsEmpty(virResctrlAllocPtr alloc) > if (alloc->mem_bw) > return false; > > + if (alloc->nmonitors) > + return false; > + > for (i = 0; i < alloc->nlevels; i++) { > virResctrlAllocPerLevelPtr a_level = alloc->levels[i]; > > @@ -2124,10 +2167,18 @@ int > virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, > const char *machinename) { > - return virResctrlDeterminePath(alloc->id, NULL, NULL, > - machinename, &alloc->path); > + if (alloc->default_group) { > + if (VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0) > + return -1; > + return 0; > + } else { > + > + return virResctrlDeterminePath(alloc->id, NULL, NULL, > + machinename, &alloc->path); > + } > } > > + > static int > virResctrlCreateGroup(virResctrlInfoPtr resctrl, > char *path) > @@ -2169,6 +2220,27 @@ virResctrlCreateGroup(virResctrlInfoPtr resctrl, > return ret; > } > > + /* In case of no explicit requirement for allocating cache and > +memory > + * bandwidth, set 'alloc->default' to 'true', then the monitoring > + * group will be created under '/sys/fs/resctrl/mon_groups' in later > + * invocation of virResctrlAllocCreate. > + * Otherwise, set 'alloc->default' to false, create a new directory > + * under '/sys/fs/resctrl/'. This is will cost a hardware 'COSID'.*/ > +static int virResctrlAllocCheckDefault(virResctrlAllocPtr alloc) { > + bool default_group = true; > + if (!alloc) > + return -1; > + > + if (alloc->nlevels) > + default_group = false; > + if (alloc->mem_bw && alloc->mem_bw->nbandwidths) > + default_group = false; > + > + alloc->default_group = default_group; > + return 0; > +} > > /* This checks if the directory for the alloc exists. If not it tries to create > * it and apply appropriate alloc settings. */ @@ -2185,6 +2257,8 @@ > virResctrlAllocCreate(virResctrlInfoPtr resctrl, > if (!alloc) > return 0; > > + virResctrlAllocCheckDefault(alloc); > + > if (virResctrlAllocDeterminePath(alloc, machinename) < 0) > return -1; > > @@ -2275,10 +2349,289 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc) > return 0; > > VIR_DEBUG("Removing resctrl allocation %s", alloc->path); > + > + while (alloc->nmonitors > 0) { > + ret = virResctrlAllocDeleteMonitor(alloc, alloc->monitors[0]->id); > + if (ret < 0) > + goto cleanup; > + } > + > if (rmdir(alloc->path) != 0 && errno != ENOENT) { > ret = -errno; > VIR_ERROR(_("Unable to remove %s (%d)"), alloc->path, errno); > } > > + ret = 0; > + cleanup: > + return ret; > +} > + > + > +static int > +virResctrlAllocGetMonitor(virResctrlAllocPtr alloc, > + const char *id, > + virResctrlAllocMonPtr *monitor, > + size_t *pos) { > + size_t i = 0; > + > + if (!alloc || !id) > + return -1; > + > + for (i = 0; i < alloc->nmonitors; i++) { > + if (alloc->monitors[i]->id && > + STREQ(id, (alloc->monitors[i])->id)) { > + if (monitor) > + *monitor = alloc->monitors[i]; > + if (pos) > + *pos = i; > + return 0; > + } > + } > + > + return -1; > +} > + > + > +int > +virResctrlAllocDetermineMonitorPath(virResctrlAllocPtr alloc, > + const char *id, > + const char *machinename) { > + virResctrlAllocMonPtr monitor = NULL; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, NULL) < 0) > + return -1; > + > + > + return virResctrlDeterminePath(monitor->id, > + alloc->path, > + "mon_groups", > + machinename, > + &monitor->path); } > + > + > +int > +virResctrlAllocAddMonitorPID(virResctrlAllocPtr alloc, > + const char *id, > + pid_t pid) { > + virResctrlAllocMonPtr monitor = NULL; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, NULL) < 0) > + return -1; > + > + return virResctrlAddPID(monitor->path, pid); } > + > + > +int > +virResctrlAllocSetMonitor(virResctrlAllocPtr alloc, > + const char *id) { > + virResctrlAllocMonPtr monitor = NULL; > + > + if (!alloc || !id) > + return - 1; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, NULL) < 0) { > + if (VIR_ALLOC(monitor) < 0) > + return -1; > + } > + > + if (VIR_STRDUP(monitor->id, (char*)id) < 0) > + return -1; > + > + if (VIR_APPEND_ELEMENT(alloc->monitors, alloc->nmonitors, monitor) < 0) > + return -1; > + > + return 0; > +} > + > +int > +virResctrlAllocCreateMonitor(virResctrlInfoPtr resctrl, > + virResctrlAllocPtr alloc, > + const char *machinename, > + const char *id) { > + virResctrlAllocMonPtr monitor = NULL; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, NULL) < 0) > + return - 1; > + > + if (virResctrlAllocDetermineMonitorPath(alloc, id, machinename) < 0) > + return -1; > + > + VIR_DEBUG("Creating resctrl monitor %s", monitor->path); > + if (virResctrlCreateGroup(resctrl, monitor->path) < 0) > + return -1; > + > + return 0; > +} > + > + > +int > +virResctrlAllocDeleteMonitor(virResctrlAllocPtr alloc, > + const char *id) { > + int ret = 0; > + > + virResctrlAllocMonPtr monitor = NULL; > + size_t pos = 0; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, &pos) < 0) > + return -1; > + > + VIR_DELETE_ELEMENT(alloc->monitors, pos, alloc->nmonitors); > + > + VIR_DEBUG("Deleting resctrl monitor %s ", monitor->path); > + if (rmdir(monitor->path) != 0 && errno != ENOENT) { > + ret = -errno; > + VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno); > + } > + > + VIR_FREE(monitor->id); > + VIR_FREE(monitor->path); > + VIR_FREE(monitor); > + return ret; > +} > + > + > +static int > +virResctrlAllocGetStatistic(virResctrlAllocPtr alloc, > + const char *id, > + const char *resfile, > + unsigned int *nnodes, > + unsigned int **nodeids, > + unsigned int **nodevals) { > + DIR *dirp = NULL; > + int ret = -1; > + int rv = -1; > + struct dirent *ent = NULL; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + char *mondatapath = NULL; > + size_t ntmpid = 0; > + size_t ntmpval = 0; > + virResctrlAllocMonPtr monitor = NULL; > + > + if (!nnodes || !nodeids || !nodevals) > + return -1; > + > + if (virResctrlAllocGetMonitor(alloc, id, &monitor, NULL) < 0) > + goto cleanup; > + > + if (!monitor || !monitor->path) > + goto cleanup; > + > + rv = virDirOpenIfExists(&dirp, monitor->path); > + if (rv <= 0) > + goto cleanup; > + > + *nnodes = 0; > + > + virBufferAsprintf(&buf, "%s/mon_data", monitor->path); > + > + mondatapath = virBufferContentAndReset(&buf); > + if (!mondatapath) > + goto cleanup; > + > + if (virDirOpen(&dirp, mondatapath) < 0) > + goto cleanup; > + > + while ((rv = virDirRead(dirp, &ent, mondatapath)) > 0) { > + char *pstrid = NULL; > + size_t i = 0; > + unsigned int len = 0; > + unsigned int counter = 0; > + unsigned int cacheid = 0; > + unsigned int cur_cacheid = 0; > + unsigned int val = 0; > + int tmpnodeid = 0; > + int tmpnodeval = 0; > + > + if (ent->d_type != DT_DIR) > + continue; > + > + /* mon_L3(|CODE|DATA)_xx, xx is cache id */ > + if (STRNEQLEN(ent->d_name, "mon_L", 5)) > + continue; > + > + len = strlen(ent->d_name); > + pstrid = ent->d_name; > + /* locating the cache id string: 'xx' */ > + for (i = 0; i < len; i++) { > + if (*(pstrid + i) == '_') > + counter ++; > + if (counter == 2) > + break; > + } > + i++; > + > + if (i >= len) > + goto cleanup; > + > + if (virStrToLong_uip(pstrid + i, NULL, 0, &cacheid) < 0) { > + VIR_DEBUG("Cannot parse id from folder '%s'", ent->d_name); > + goto cleanup; > + } > + > + rv = virFileReadValueUint(&val, > + "%s/%s/%s", > + mondatapath, ent->d_name, resfile); > + > + if (rv == -2) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("file %s/%s/%s does not exist"), > + mondatapath, ent->d_name, resfile); > + goto cleanup; > + } else { > + if (rv < 0) > + goto cleanup; > + } > + > + /* The ultimate caller will be responiblefor free memory of > + * 'nodeids' an 'nodevals' */ > + if (VIR_APPEND_ELEMENT(*nodeids, ntmpid, cacheid) < 0) > + goto cleanup; > + if (VIR_APPEND_ELEMENT(*nodevals, ntmpval, val) < 0) > + goto cleanup; > + > + cur_cacheid = ntmpval - 1; > + /* sort the cache information in caach bank id's ascending order */ > + for (i = 0; i < cur_cacheid; i++) { > + if ((*nodeids)[cur_cacheid] < (*nodeids)[i]) { > + tmpnodeid = (*nodeids)[cur_cacheid]; > + tmpnodeval = (*nodevals)[cur_cacheid]; > + (*nodeids)[cur_cacheid] = (*nodeids)[i]; > + (*nodevals)[cur_cacheid] = (*nodevals)[i]; > + (*nodeids)[i] = tmpnodeid; > + (*nodevals)[i] = tmpnodeval; > + } > + } > + } > + > + (*nnodes) = ntmpval; > + ret = 0; > + cleanup: > + VIR_FREE(mondatapath); > + VIR_DIR_CLOSE(dirp); > + return ret; > +} > + > + > +int > +virResctrlAllocGetCacheOccupancy(virResctrlAllocPtr alloc, > + const char *id, > + unsigned int *nbank, > + unsigned int **bankids, > + unsigned int **bankcaches) { > + int ret = - 1; > + > + ret = virResctrlAllocGetStatistic(alloc, id, "llc_occupancy", > + nbank, bankids, bankcaches); > + > return ret; > } > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index > 51bb68b..0f63997 100644 > --- a/src/util/virresctrl.h > +++ b/src/util/virresctrl.h > @@ -160,4 +160,35 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, > int virResctrlAllocRemove(virResctrlAllocPtr alloc); > > +int > +virResctrlAllocDetermineMonitorPath(virResctrlAllocPtr alloc, > + const char *id, > + const char *machinename); > + > +int > +virResctrlAllocAddMonitorPID(virResctrlAllocPtr alloc, > + const char *id, > + pid_t pid); > + > +int > +virResctrlAllocSetMonitor(virResctrlAllocPtr alloc, > + const char *id); > + > +int > +virResctrlAllocCreateMonitor(virResctrlInfoPtr resctrl, > + virResctrlAllocPtr alloc, > + const char *machinename, > + const char *id); > + > +int > +virResctrlAllocDeleteMonitor(virResctrlAllocPtr alloc, > + const char *id); > + > +int > +virResctrlAllocGetCacheOccupancy(virResctrlAllocPtr alloc, > + const char *id, > + unsigned int *nbank, > + unsigned int **bankids, > + unsigned int **bankcaches); > + > #endif /* __VIR_RESCTRL_H__ */ >