[libvirt] [PATCH v2 0/3] add per cpu stats to all domain stats
Info provided in virDomainGetCPUStats is currently missed in all domain stats. This patch removes this discrepancy. You may need not yet merged patch 'qemu: fix crash on getting block stats for empty cdrom' to test this series. diff from v1: ================ 1. reuse code (patches 1, 2) 2. move per cpu stats to a distinct stats group 3. add documentation Nikolay Shirokovskiy (3): cgroup: extract interface part from virCgroupGetPercpuStats cgroup: reuse virCgroupGetPercpuTime in virCgroupGetPercpuVcpuSum add per cpu stats to all domain stats docs/news.xml | 9 ++ include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 7 ++ src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 64 ++++++++++++++ src/util/vircgroup.c | 178 +++++++++++++++++++++++++++------------ src/util/vircgroup.h | 17 ++++ tools/virsh-domain-monitor.c | 7 ++ tools/virsh.pod | 11 ++- 9 files changed, 238 insertions(+), 58 deletions(-) -- 1.8.3.1
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 = ¶ms[(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(¶ms[(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__ */ -- 1.8.3.1
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 = ¶ms[(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(¶ms[(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__ */
After trying a while on this path I decided not to push code reuse further in this patch. Actually it is quite difficult to reuse further without fragmenting function too much. So let's stay with this patch series. On 02.02.2017 17:08, Nikolay Shirokovskiy wrote:
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 = ¶ms[(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(¶ms[(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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
--- src/util/vircgroup.c | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e4eaf32..06dd589 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -89,6 +89,10 @@ typedef enum { */ } virCgroupFlags; +static int +virCgroupGetPercpuTime(virCgroupPtr group, + virBitmapPtr cpumap, + unsigned long long *time); /** * virCgroupGetDevicePermsString: @@ -3123,49 +3127,41 @@ virCgroupDenyDevicePath(virCgroupPtr group, * s3 = t03 + t13 */ static int -virCgroupGetPercpuVcpuSum(virCgroupPtr group, - virBitmapPtr guestvcpus, - unsigned long long *sum_cpu_time, - size_t nsum, - virBitmapPtr cpumap) +virCgroupGetPercpuVTime(virCgroupPtr group, + virBitmapPtr guestvcpus, + unsigned long long *vtime, + virBitmapPtr cpumap) { int ret = -1; ssize_t i = -1; - char *buf = NULL; virCgroupPtr group_vcpu = NULL; + unsigned long long *time = NULL; + int ncpus = virBitmapSize(cpumap); + + + if (VIR_ALLOC_N(time, ncpus) < 0) + goto cleanup; while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { - char *pos; - unsigned long long tmp; ssize_t j; if (virCgroupNewThread(group, VIR_CGROUP_THREAD_VCPU, i, false, &group_vcpu) < 0) goto cleanup; - if (virCgroupGetCpuacctPercpuUsage(group_vcpu, &buf) < 0) + if (virCgroupGetPercpuTime(group_vcpu, cpumap, time) < 0) goto cleanup; - pos = buf; - for (j = virBitmapNextSetBit(cpumap, -1); - j >= 0 && j < nsum; - j = virBitmapNextSetBit(cpumap, j)) { - if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cpuacct parse error")); - goto cleanup; - } - sum_cpu_time[j] += tmp; - } + for (j = 0; j < ncpus; j++) + vtime[j] += time[j]; virCgroupFree(&group_vcpu); - VIR_FREE(buf); } ret = 0; cleanup: virCgroupFree(&group_vcpu); - VIR_FREE(buf); + VIR_FREE(time); return ret; } @@ -3254,8 +3250,8 @@ virCgroupGetCpuStats(virCgroupPtr group, if (VIR_ALLOC_N(stats->vtime, ncpus) < 0) goto cleanup; - if (virCgroupGetPercpuVcpuSum(group, guestvcpus, stats->vtime, - ncpus, cpumap) < 0) + if (virCgroupGetPercpuVTime(group, guestvcpus, stats->vtime, + cpumap) < 0) goto cleanup; } -- 1.8.3.1
Add per host cpu info provided in virDomainGetCPUStats to the stats provided in virConnectGetAllDomainStats. Namely: "cpu.count" - number of host cpus "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain in nanoseconds --- docs/news.xml | 9 ++++++ include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 7 +++++ src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-domain-monitor.c | 7 +++++ tools/virsh.pod | 11 +++++-- 6 files changed, 97 insertions(+), 2 deletions(-) diff --git a/docs/news.xml b/docs/news.xml index f408293..6e40c33 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -42,6 +42,15 @@ Allow setting MTU size for some types of domain interface. </description> </change> + <change> + <summary> + Show per host cpu stats in all domain stats + </summary> + <description> + Show stats provided in virDomainGetCPUStats in all domain stats + as well. + </description> + </change> </section> <section title="Bug fixes"> <change> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e303140..2691ebe 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2020,6 +2020,7 @@ typedef enum { VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */ VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */ VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */ + VIR_DOMAIN_STATS_PER_CPU = (1 << 7), /* return domain per host CPU info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5b3e842..3726938 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11126,6 +11126,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.system" - system cpu time spent in nanoseconds as unsigned long * long. * + * VIR_DOMAIN_STATS_PER_CPU: + * Return per host CPU statistics + * "cpu.count" - number of host cpus + * "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds + * "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain + * in nanoseconds + * * VIR_DOMAIN_STATS_BALLOON: * Return memory balloon device information. * The typed parameter keys are in this format: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 088f55e..d457a71 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18806,6 +18806,69 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, } static int +qemuDomainGetStatsPerCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv = dom->privateData; + virCgroupCpuStats stats = {0}; + virBitmapPtr guestvcpus = NULL; + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + int ncpus; + size_t i; + int ret = -1; + + if (qemuDomainHasVcpuPids(dom)) + guestvcpus = virDomainDefGetOnlineVcpumap(dom->def); + + ncpus = virCgroupGetCpuStats(priv->cgroup, guestvcpus, &stats); + + if (ncpus > 0) { + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "cpu.count", + ncpus) < 0) + goto cleanup; + + for (i = 0; i < ncpus; i++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%zu.time", i); + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + param_name, + stats.time[i]) < 0) + goto cleanup; + + + if (stats.vtime) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%zu.vtime", i); + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + param_name, + stats.vtime[i]) < 0) + goto cleanup; + } + } + } + + ret = 0; + + cleanup: + if (!ret && virGetLastError()) + virResetLastError(); + virBitmapFree(guestvcpus); + virCgroupCpuStatsFree(&stats); + + return ret; +} + +static int qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, @@ -19381,6 +19444,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, + { qemuDomainGetStatsPerCpu, VIR_DOMAIN_STATS_PER_CPU, false }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false }, diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 77aa272..7707a96 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1953,6 +1953,10 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("report domain physical cpu usage"), }, + {.name = "per-cpu", + .type = VSH_OT_BOOL, + .help = N_("report domain per physical cpu usage"), + }, {.name = "balloon", .type = VSH_OT_BOOL, .help = N_("report domain balloon statistics"), @@ -2072,6 +2076,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "cpu-total")) stats |= VIR_DOMAIN_STATS_CPU_TOTAL; + if (vshCommandOptBool(cmd, "per-cpu")) + stats |= VIR_DOMAIN_STATS_PER_CPU; + if (vshCommandOptBool(cmd, "balloon")) stats |= VIR_DOMAIN_STATS_BALLOON; diff --git a/tools/virsh.pod b/tools/virsh.pod index a470409..c10a4ee 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -860,8 +860,9 @@ I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. =item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>] -[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>] -[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] +[I<--cpu-total>] [I<--per-cpu>] [I<--balloon>] [I<--vcpu>] [I<--interface>] +[I<--block>] [I<--perf>] +[[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] [I<--list-transient>] [I<--list-running>] [I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] @@ -894,6 +895,12 @@ I<--cpu-total> returns: "cpu.user" - user cpu time spent in nanoseconds, "cpu.system" - system cpu time spent in nanoseconds +I<--per-cpu> returns: +"cpu.count" - number of host cpus, +"cpu.<num>.time" - total cpu time spent for this domain in nanoseconds, +"cpu.<num>.vtime" - time spent in virtual cpu threads for this domain +in nanoseconds. + I<--balloon> returns: "balloon.current" - the memory in kiB currently used, "balloon.maximum" - the maximum memory in kiB allowed, -- 1.8.3.1
On Thu, Feb 02, 2017 at 14:09:33 +0300, Nikolay Shirokovskiy wrote:
Add per host cpu info provided in virDomainGetCPUStats to the stats provided in virConnectGetAllDomainStats. Namely:
"cpu.count" - number of host cpus "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain in nanoseconds --- docs/news.xml | 9 ++++++ include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 7 +++++ src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-domain-monitor.c | 7 +++++ tools/virsh.pod | 11 +++++-- 6 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/docs/news.xml b/docs/news.xml index f408293..6e40c33 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -42,6 +42,15 @@ Allow setting MTU size for some types of domain interface. </description> </change> + <change> + <summary> + Show per host cpu stats in all domain stats + </summary> + <description> + Show stats provided in virDomainGetCPUStats in all domain stats + as well. + </description> + </change> </section> <section title="Bug fixes"> <change>
Please don't put the news file update into the same patch as the code. We did not write this rule down but in general the conflicts when backporting any patch should not have to deal with the news file.
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e303140..2691ebe 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2020,6 +2020,7 @@ typedef enum { VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */ VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */ VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */ + VIR_DOMAIN_STATS_PER_CPU = (1 << 7), /* return domain per host CPU info */ } virDomainStatsTypes;
typedef enum { diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5b3e842..3726938 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11126,6 +11126,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.system" - system cpu time spent in nanoseconds as unsigned long * long. * + * VIR_DOMAIN_STATS_PER_CPU:
This new stats section (which I don't really think it's necessary at all) shares the prefix with the existing one which is confusing. Since I don't see a reason for this section and you did not write anything to explain why it's necessary just remove it and put everything in the existing section.
+ * Return per host CPU statistics + * "cpu.count" - number of host cpus + * "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds + * "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain + * in nanoseconds
Sooo, these are host cpu numbers, right?
+ * * VIR_DOMAIN_STATS_BALLOON: * Return memory balloon device information. * The typed parameter keys are in this format: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 088f55e..d457a71 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18806,6 +18806,69 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, }
static int +qemuDomainGetStatsPerCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv = dom->privateData; + virCgroupCpuStats stats = {0}; + virBitmapPtr guestvcpus = NULL; + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + int ncpus; + size_t i; + int ret = -1; + + if (qemuDomainHasVcpuPids(dom)) + guestvcpus = virDomainDefGetOnlineVcpumap(dom->def);
Does it make sense to do this if we don't have the PIDs? Without that it's usually one thread anyways. Peter
On 03.02.2017 19:09, Peter Krempa wrote:
On Thu, Feb 02, 2017 at 14:09:33 +0300, Nikolay Shirokovskiy wrote:
Add per host cpu info provided in virDomainGetCPUStats to the stats provided in virConnectGetAllDomainStats. Namely:
"cpu.count" - number of host cpus "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain in nanoseconds --- docs/news.xml | 9 ++++++ include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 7 +++++ src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-domain-monitor.c | 7 +++++ tools/virsh.pod | 11 +++++-- 6 files changed, 97 insertions(+), 2 deletions(-)
[snip]
static int +qemuDomainGetStatsPerCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv = dom->privateData; + virCgroupCpuStats stats = {0}; + virBitmapPtr guestvcpus = NULL; + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + int ncpus; + size_t i; + int ret = -1; + + if (qemuDomainHasVcpuPids(dom)) + guestvcpus = virDomainDefGetOnlineVcpumap(dom->def);
Does it make sense to do this if we don't have the PIDs? Without that it's usually one thread anyways.
It is on par with qemuDomainGetCPUStats. I don't quite understand you. You mean even if not qemuDomainHasVcpuPids we can gather cpu.<num>.vtime? Nikolay
participants (2)
-
Nikolay Shirokovskiy -
Peter Krempa