Re: [libvirt] [PATCH v2 1/3] cgroup: extract interface part from virCgroupGetPercpuStats

After refactorings of this patch virCgroupGetPercpuStats has little to do with cgroups. As already mentioned in commit message it mostly adapts raw cpu data to storage used in API. So I want to move this function elsewhere together with virCgroupCpuStatsPtr structure. I just can find proper header/source to use and prefix too. Actually I want to reuse this adapting function in vz driver in the process of implementing virDomainGetCPUStats and don't want to include vircgroup.h there. Any ideas? On 02.02.2017 14:09, Nikolay Shirokovskiy wrote: > virCgroupGetPercpuStats do 2 things. First it extracts per cpu stats > from cgroups, second it puts stats values into virTypedParameterPtr in > accordance with virDomainGetCPUStats interface. As we need first > function in order to prodive per cpus stats in virConnectGetAllDomainStats > lets split these two functions. > --- > src/libvirt_private.syms | 2 + > src/util/vircgroup.c | 140 +++++++++++++++++++++++++++++++++++------------ > src/util/vircgroup.h | 17 ++++++ > 3 files changed, 124 insertions(+), 35 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 8e994c7..e05335e 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1312,6 +1312,7 @@ virCgroupBindMount; > virCgroupControllerAvailable; > virCgroupControllerTypeFromString; > virCgroupControllerTypeToString; > +virCgroupCpuStatsFree; > virCgroupDelThread; > virCgroupDenyAllDevices; > virCgroupDenyDevice; > @@ -1335,6 +1336,7 @@ virCgroupGetCpusetCpus; > virCgroupGetCpusetMemoryMigrate; > virCgroupGetCpusetMems; > virCgroupGetCpuShares; > +virCgroupGetCpuStats; > virCgroupGetDevicePermsString; > virCgroupGetDomainTotalCpuStats; > virCgroupGetFreezerState; > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 5aa1db5..e4eaf32 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -3170,6 +3170,105 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, > } > > > +void virCgroupCpuStatsFree(virCgroupCpuStatsPtr stats) > +{ > + VIR_FREE(stats->time); > + VIR_FREE(stats->vtime); > +} > + > + > +/* > + * Parses sparse per cpu usage cgroup output into continuos array. > + * Sparse map is given by @cpumap. > + */ > +static int > +virCgroupGetPercpuTime(virCgroupPtr group, > + virBitmapPtr cpumap, > + unsigned long long *time) > +{ > + char *pos; > + char *buf = NULL; > + size_t i; > + int ret = -1; > + int ncpus = virBitmapSize(cpumap); > + > + memset(time, 0, sizeof(*time) * ncpus); > + > + /* we get percpu cputime accounting info. */ > + if (virCgroupGetCpuacctPercpuUsage(group, &buf)) > + goto cleanup; > + pos = buf; > + > + for (i = 0; i < ncpus; i++) { > + if (!virBitmapIsBitSet(cpumap, i)) > + continue; > + > + if (virStrToLong_ull(pos, &pos, 10, &time[i]) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("cpuacct parse error")); > + goto cleanup; > + } > + } > + > + ret = 0; > + > + cleanup: > + VIR_FREE(buf); > + return ret; > +} > + > + > +/* > + * Get per cpu stats for given domain group. > + * If @guestvcpus is not NULL per cpu stats for virtual cpu threads > + * are provided as well. > + */ > +int > +virCgroupGetCpuStats(virCgroupPtr group, > + virBitmapPtr guestvcpus, > + virCgroupCpuStatsPtr stats) > +{ > + int ret = -1; > + int ncpus; > + virBitmapPtr cpumap = NULL; > + > + /* To parse account file, we need to know how many cpus are present. */ > + if (!(cpumap = virHostCPUGetPresentBitmap())) > + return -1; > + > + ncpus = virBitmapSize(cpumap); > + if (ncpus == 0) { > + ret = 0; > + goto cleanup; > + } > + > + memset(stats, 0, sizeof(*stats)); > + > + if (VIR_ALLOC_N(stats->time, ncpus) < 0) > + goto cleanup; > + > + if (virCgroupGetPercpuTime(group, cpumap, stats->time) < 0) > + goto cleanup; > + > + if (guestvcpus) { > + if (VIR_ALLOC_N(stats->vtime, ncpus) < 0) > + goto cleanup; > + > + if (virCgroupGetPercpuVcpuSum(group, guestvcpus, stats->vtime, > + ncpus, cpumap) < 0) > + goto cleanup; > + } > + > + ret = ncpus; > + > + cleanup: > + if (ret < 0) > + virCgroupCpuStatsFree(stats); > + virBitmapFree(cpumap); > + return ret; > +} > + > + > /** > * virCgroupGetPercpuStats: > * @cgroup: cgroup data structure > @@ -3201,13 +3300,9 @@ virCgroupGetPercpuStats(virCgroupPtr group, > int ret = -1; > size_t i; > int need_cpus, total_cpus; > - char *pos; > - char *buf = NULL; > - unsigned long long *sum_cpu_time = NULL; > virTypedParameterPtr ent; > int param_idx; > - unsigned long long cpu_time; > - virBitmapPtr cpumap = NULL; > + virCgroupCpuStats stats = {0}; > > /* return the number of supported params */ > if (nparams == 0 && ncpus != 0) { > @@ -3217,12 +3312,9 @@ virCgroupGetPercpuStats(virCgroupPtr group, > return CGROUP_NB_PER_CPU_STAT_PARAM + 1; > } > > - /* To parse account file, we need to know how many cpus are present. */ > - if (!(cpumap = virHostCPUGetPresentBitmap())) > + if ((total_cpus = virCgroupGetCpuStats(group, guestvcpus, &stats)) < 0) > return -1; > > - total_cpus = virBitmapSize(cpumap); > - > /* return total number of cpus */ > if (ncpus == 0) { > ret = total_cpus; > @@ -3236,30 +3328,16 @@ virCgroupGetPercpuStats(virCgroupPtr group, > goto cleanup; > } > > - /* we get percpu cputime accounting info. */ > - if (virCgroupGetCpuacctPercpuUsage(group, &buf)) > - goto cleanup; > - pos = buf; > - > /* return percpu cputime in index 0 */ > param_idx = 0; > > /* number of cpus to compute */ > need_cpus = MIN(total_cpus, start_cpu + ncpus); > > - for (i = 0; i < need_cpus; i++) { > - if (!virBitmapIsBitSet(cpumap, i)) { > - cpu_time = 0; > - } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("cpuacct parse error")); > - goto cleanup; > - } > - if (i < start_cpu) > - continue; > + for (i = start_cpu; i < need_cpus; i++) { > ent = &params[(i - start_cpu) * nparams + param_idx]; > if (virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, > - VIR_TYPED_PARAM_ULLONG, cpu_time) < 0) > + VIR_TYPED_PARAM_ULLONG, stats.time[i]) < 0) > goto cleanup; > } > > @@ -3267,18 +3345,12 @@ virCgroupGetPercpuStats(virCgroupPtr group, > param_idx = 1; > > if (guestvcpus && param_idx < nparams) { > - if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) > - goto cleanup; > - if (virCgroupGetPercpuVcpuSum(group, guestvcpus, sum_cpu_time, > - need_cpus, cpumap) < 0) > - goto cleanup; > - > for (i = start_cpu; i < need_cpus; i++) { > if (virTypedParameterAssign(&params[(i - start_cpu) * nparams + > param_idx], > VIR_DOMAIN_CPU_STATS_VCPUTIME, > VIR_TYPED_PARAM_ULLONG, > - sum_cpu_time[i]) < 0) > + stats.vtime[i]) < 0) > goto cleanup; > } > > @@ -3288,9 +3360,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, > ret = param_idx; > > cleanup: > - virBitmapFree(cpumap); > - VIR_FREE(sum_cpu_time); > - VIR_FREE(buf); > + virCgroupCpuStatsFree(&stats); > return ret; > } > > diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h > index 2de1bf2..40b420d 100644 > --- a/src/util/vircgroup.h > +++ b/src/util/vircgroup.h > @@ -297,4 +297,21 @@ int virCgroupSetOwner(virCgroupPtr cgroup, > int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); > > bool virCgroupControllerAvailable(int controller); > + > +struct virCgroupCpuStats_ { > + /* time arrays are not sparsed, that is if cpu is offline > + * it has value 0 for time instead of being skipped */ > + unsigned long long *time; /* per cpu time */ > + unsigned long long *vtime;/* per cpu time for virtual cpu threads */ > +}; > + > +typedef struct virCgroupCpuStats_ virCgroupCpuStats; > +typedef virCgroupCpuStats *virCgroupCpuStatsPtr; > + > +int virCgroupGetCpuStats(virCgroupPtr group, > + virBitmapPtr guestvcpus, > + virCgroupCpuStatsPtr stats); > +void virCgroupCpuStatsFree(virCgroupCpuStatsPtr stats); > + > + > #endif /* __VIR_CGROUP_H__ */ > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list