[libvirt] [PATCH] bhyve: implement domainGetCPUStats

For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats. For total CPU stats, add virBhyveGetDomainTotalCpuStats() that gets the hypervisor process CPU stats using kvm (kernel memory interface). --- configure.ac | 7 +++ src/bhyve/bhyve_driver.c | 38 +++++++++++++ src/bhyve/bhyve_process.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++ src/bhyve/bhyve_process.h | 10 ++++ 4 files changed, 191 insertions(+) diff --git a/configure.ac b/configure.ac index ea85851..dbfada2 100644 --- a/configure.ac +++ b/configure.ac @@ -2678,6 +2678,13 @@ AC_CHECK_DECLS([cpuset_getaffinity], #include <sys/cpuset.h> ]) +# Check for BSD kvm (kernel memory interface) +if test $with_freebsd = yes; then + AC_CHECK_LIB([kvm], [kvm_getprocs], [], + [AC_MSG_ERROR([BSD kernel memory interface library is required to build on FreeBSD])] + ) +fi + # Check if we need to look for ifconfig if test "$want_ifconfig" = "yes"; then AC_PATH_PROG([IFCONFIG_PATH], [ifconfig]) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 0cafe4c..25b0dce 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -930,6 +930,43 @@ bhyveDomainGetMetadata(virDomainPtr dom, } static int +bhyveDomainGetCPUStats(virDomainPtr dom, + virTypedParameterPtr params ATTRIBUTE_UNUSED, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + if (!(vm = bhyveDomObjFromDomain(dom))) + return ret; + + if (virDomainGetCPUStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (start_cpu == -1) + ret = virBhyveGetDomainTotalCpuStats(vm, params, nparams); + else + ret = virBhyveGetDomainPercpuStats(vm, params, nparams, + start_cpu, ncpus); + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int bhyveNodeGetCPUStats(virConnectPtr conn, int cpuNum, virNodeCPUStatsPtr params, @@ -1198,6 +1235,7 @@ static virDriver bhyveDriver = { .domainOpenConsole = bhyveDomainOpenConsole, /* 1.2.4 */ .domainSetMetadata = bhyveDomainSetMetadata, /* 1.2.4 */ .domainGetMetadata = bhyveDomainGetMetadata, /* 1.2.4 */ + .domainGetCPUStats = bhyveDomainGetCPUStats, /* 1.2.4 */ .nodeGetCPUStats = bhyveNodeGetCPUStats, /* 1.2.2 */ .nodeGetMemoryStats = bhyveNodeGetMemoryStats, /* 1.2.2 */ .nodeGetInfo = bhyveNodeGetInfo, /* 1.2.3 */ diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index a557bc5..e1f4324 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -22,7 +22,11 @@ #include <config.h> #include <fcntl.h> +#include <kvm.h> +#include <sys/param.h> #include <sys/types.h> +#include <sys/sysctl.h> +#include <sys/user.h> #include <sys/ioctl.h> #include <net/if.h> #include <net/if_tap.h> @@ -41,11 +45,15 @@ #include "virnetdev.h" #include "virnetdevbridge.h" #include "virnetdevtap.h" +#include "virtypedparam.h" #define VIR_FROM_THIS VIR_FROM_BHYVE VIR_LOG_INIT("bhyve.bhyve_process"); +#define BHYVE_TOTAL_CPU_STAT_PARAM 1 +#define BHYVE_PER_CPU_STAT_PARAM 1 + static virDomainObjPtr bhyveProcessAutoDestroy(virDomainObjPtr vm, virConnectPtr conn ATTRIBUTE_UNUSED, @@ -246,3 +254,131 @@ virBhyveProcessStop(bhyveConnPtr driver, virCommandFree(cmd); return ret; } + +static int +virBhyveExtractCpuTotal(char **const groups, + void *opaque) +{ + return virStrToLong_ull(groups[0], NULL, 10, opaque); +} + +int +virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm, + virTypedParameterPtr params ATTRIBUTE_UNUSED, + unsigned int nparams) +{ + struct kinfo_proc* kp; + kvm_t* kd; + char errbuf[_POSIX2_LINE_MAX]; + int nprocs; + int ret = -1; + + if (nparams == 0) + return BHYVE_TOTAL_CPU_STAT_PARAM; + + if ((kd = kvm_openfiles(NULL, NULL, NULL, O_RDONLY, errbuf)) == NULL) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("Unable to get kvm descriptor: %s"), + errbuf); + return -1; + + } + + kp = kvm_getprocs(kd, KERN_PROC_PID, vm->pid, &nprocs); + if (kp == NULL || nprocs != 1) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("Unable to obtain information about pid: %d"), + (int)vm->pid); + goto cleanup; + } + + if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, + kp->ki_runtime * 1000ul) < 0) + goto cleanup; + + ret = nparams; + + cleanup: + kvm_close(kd); + + return ret; +} + +#define TSC_TO_NSEC(x) (unsigned long long)((double)x * (1000.0 * 1000.0 * 1000.0 / (double)tsc_freq)) + +int +virBhyveGetDomainPercpuStats(virDomainObjPtr vm, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus) +{ + virCommandPtr cmd = NULL; + int need_cpus, ret = -1; + size_t i, tsc_freq_size; + unsigned long long tsc_freq; + + if (nparams == 0 && ncpus != 0) + return BHYVE_PER_CPU_STAT_PARAM; + + if (ncpus == 0) + return vm->def->vcpus; + + if (start_cpu >= vm->def->vcpus) { + virReportError(VIR_ERR_INVALID_ARG, + _("start_cpu %d larger than maximum of %d"), + start_cpu, vm->def->vcpus - 1); + goto cleanup; + } + + need_cpus = MIN(vm->def->vcpus, start_cpu + ncpus); + + tsc_freq_size = sizeof(tsc_freq); + if (sysctlbyname("machdep.tsc_freq", &tsc_freq, &tsc_freq_size, NULL, 0) < 0) { + virReportSystemError(errno, + _("sysctl failed for '%s'"), + "machdep.tsc_freq"); + return -1; + } + + for (i = start_cpu; i < need_cpus; i++) { + unsigned long long cpu_stats; + + const char *regexes[] = { + "^vcpu total runtime\\s+([0-9]+)$" + }; + int vars[] = { + 1, + }; + + cmd = virCommandNew(BHYVECTL); + virCommandAddArgPair(cmd, "--vm", vm->def->name); + virCommandAddArg(cmd, "--get-stats"); + virCommandAddArgFormat(cmd, "--cpu=%zu", i); + + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virBhyveExtractCpuTotal, + &cpu_stats, + NULL) < 0) + goto cleanup; + + if (virTypedParameterAssign(¶ms[(i - start_cpu) * nparams], + VIR_DOMAIN_CPU_STATS_VCPUTIME, + VIR_TYPED_PARAM_ULLONG, + TSC_TO_NSEC(cpu_stats)) < 0) + goto cleanup; + + virCommandFree(cmd); + cmd = NULL; + } + + ret = nparams; + + cleanup: + virCommandFree(cmd); + return ret; +} diff --git a/src/bhyve/bhyve_process.h b/src/bhyve/bhyve_process.h index f91504e..2cc577a 100644 --- a/src/bhyve/bhyve_process.h +++ b/src/bhyve/bhyve_process.h @@ -34,6 +34,16 @@ int virBhyveProcessStop(bhyveConnPtr driver, virDomainObjPtr vm, virDomainShutoffReason reason); +int virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm, + virTypedParameterPtr params, + unsigned int nparams); + +int virBhyveGetDomainPercpuStats(virDomainObjPtr vm, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus); + typedef enum { VIR_BHYVE_PROCESS_START_AUTODESTROY = 1 << 0, } bhyveProcessStartFlags; -- 1.9.0

Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
For total CPU stats, add virBhyveGetDomainTotalCpuStats() that gets the hypervisor process CPU stats using kvm (kernel memory interface). --- configure.ac | 7 +++ src/bhyve/bhyve_driver.c | 38 +++++++++++++ src/bhyve/bhyve_process.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++ src/bhyve/bhyve_process.h | 10 ++++ 4 files changed, 191 insertions(+) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index a557bc5..e1f4324 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c
...
+int +virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm, + virTypedParameterPtr params ATTRIBUTE_UNUSED, + unsigned int nparams) +{ + struct kinfo_proc* kp; + kvm_t* kd;
Just noticed that it should be: struct kinfo_proc *kp; kvm_t *kd; I will not re-send the patch just because of that and will include the fix in the next version.
+ char errbuf[_POSIX2_LINE_MAX]; + int nprocs; + int ret = -1;
Roman Bogorodskiy

On 04/22/2014 07:44 PM, Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
In virDomainGetCPUStats API, the start_cpu and ncpus parameters refer to host CPUs, not the vcpus. Jan

Ján Tomko wrote:
On 04/22/2014 07:44 PM, Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
In virDomainGetCPUStats API, the start_cpu and ncpus parameters refer to host CPUs, not the vcpus.
Looks like I misunderstood the API. :-( In other words, this API operates with the host CPUs? So if user calls it with e.g. start_cpu = 2 and ncpus = 2, I should return info on how much time host CPUs 2 and 3 spend on the given domain (as the opposite to how much time domain's vcpu 2 and vcpu 3 were busy)? Roman Bogorodskiy

On 04/24/2014 11:14 AM, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On 04/22/2014 07:44 PM, Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
In virDomainGetCPUStats API, the start_cpu and ncpus parameters refer to host CPUs, not the vcpus.
Looks like I misunderstood the API. :-(
In other words, this API operates with the host CPUs? So if user calls it with e.g. start_cpu = 2 and ncpus = 2, I should return info on how much time host CPUs 2 and 3 spend on the given domain (as the opposite to how much time domain's vcpu 2 and vcpu 3 were busy)?
Yes. Jan

Ján Tomko wrote:
On 04/24/2014 11:14 AM, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On 04/22/2014 07:44 PM, Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
In virDomainGetCPUStats API, the start_cpu and ncpus parameters refer to host CPUs, not the vcpus.
Looks like I misunderstood the API. :-(
In other words, this API operates with the host CPUs? So if user calls it with e.g. start_cpu = 2 and ncpus = 2, I should return info on how much time host CPUs 2 and 3 spend on the given domain (as the opposite to how much time domain's vcpu 2 and vcpu 3 were busy)?
Yes.
Jan
Looks like FreeBSD does not support such kind of per-cpu reporting for host cpus. Just to make sure I didn't miss something, I asked that question of the FreeBSD virtualization maillist and looks like I'm right about that [1]. Would it better to abandon this feature for now and wait when it'd be supported (I'm not sure if/when it's going to happen), or just leave the code that reports only the stats for all cpus? 1: http://lists.freebsd.org/pipermail/freebsd-virtualization/2014-April/002477.... Roman Bogorodskiy

On Tue, Apr 29, 2014 at 08:54:26PM +0400, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On 04/24/2014 11:14 AM, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On 04/22/2014 07:44 PM, Roman Bogorodskiy wrote:
For per CPU stats, implement virBhyveGetDomainPercpuStats() that uses bhyvectl tool to obtain the guest's vcpu stats.
In virDomainGetCPUStats API, the start_cpu and ncpus parameters refer to host CPUs, not the vcpus.
Looks like I misunderstood the API. :-(
In other words, this API operates with the host CPUs? So if user calls it with e.g. start_cpu = 2 and ncpus = 2, I should return info on how much time host CPUs 2 and 3 spend on the given domain (as the opposite to how much time domain's vcpu 2 and vcpu 3 were busy)?
Yes.
Jan
Looks like FreeBSD does not support such kind of per-cpu reporting for host cpus. Just to make sure I didn't miss something, I asked that question of the FreeBSD virtualization maillist and looks like I'm right about that [1].
Would it better to abandon this feature for now and wait when it'd be supported (I'm not sure if/when it's going to happen), or just leave the code that reports only the stats for all cpus?
I think it is better to leave it unsupported, so apps can see the VIR_ERR_NO_SUPPORT error and not mistakenly interpret incorrect data. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/30/2014 12:03 PM, Daniel P. Berrange wrote:
On Tue, Apr 29, 2014 at 08:54:26PM +0400, Roman Bogorodskiy wrote:
Looks like FreeBSD does not support such kind of per-cpu reporting for host cpus. Just to make sure I didn't miss something, I asked that question of the FreeBSD virtualization maillist and looks like I'm right about that [1].
Would it better to abandon this feature for now and wait when it'd be supported (I'm not sure if/when it's going to happen), or just leave the code that reports only the stats for all cpus?
I think it is better to leave it unsupported, so apps can see the VIR_ERR_NO_SUPPORT error and not mistakenly interpret incorrect data.
The apps should check how many stat parameters are supported for a given pCPU by calling virDomainGetCPUStats with nparams=0 and/or check the actual number of stats returned. Could we return valid results for total CPU stats (start_cpu = -1) and just not return any for all other start_cpu values? From the API documentation [1]: * Typical use sequence is below. * * getting total stats: set start_cpu as -1, ncpus 1 * virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0) => nparams * params = calloc(nparams, sizeof(virTypedParameter)) * virDomainGetCPUStats(dom, params, nparams, -1, 1, 0) => total stats. * * getting per-cpu stats: * virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0) => ncpus * virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0) => nparams * params = calloc(ncpus * nparams, sizeof(virTypedParameter)) * virDomainGetCPUStats(dom, params, nparams, 0, ncpus, 0) => per-cpu stats Jan [1] http://libvirt.org/html/libvirt-libvirt.html#virDomainGetCPUStats

On Wed, Apr 30, 2014 at 02:51:18PM +0200, Ján Tomko wrote:
On 04/30/2014 12:03 PM, Daniel P. Berrange wrote:
On Tue, Apr 29, 2014 at 08:54:26PM +0400, Roman Bogorodskiy wrote:
Looks like FreeBSD does not support such kind of per-cpu reporting for host cpus. Just to make sure I didn't miss something, I asked that question of the FreeBSD virtualization maillist and looks like I'm right about that [1].
Would it better to abandon this feature for now and wait when it'd be supported (I'm not sure if/when it's going to happen), or just leave the code that reports only the stats for all cpus?
I think it is better to leave it unsupported, so apps can see the VIR_ERR_NO_SUPPORT error and not mistakenly interpret incorrect data.
The apps should check how many stat parameters are supported for a given pCPU by calling virDomainGetCPUStats with nparams=0 and/or check the actual number of stats returned.
Could we return valid results for total CPU stats (start_cpu = -1) and just not return any for all other start_cpu values?
Wouldn't that just be duplicating what we already provide via the virDomainGetInfo cpuTime field ? If so it doesn't seem like we need to add this feature to virDomainGetCPUStats too ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/30/2014 02:57 PM, Daniel P. Berrange wrote:
On Wed, Apr 30, 2014 at 02:51:18PM +0200, Ján Tomko wrote:
The apps should check how many stat parameters are supported for a given pCPU by calling virDomainGetCPUStats with nparams=0 and/or check the actual number of stats returned.
Could we return valid results for total CPU stats (start_cpu = -1) and just not return any for all other start_cpu values?
Wouldn't that just be duplicating what we already provide via the virDomainGetInfo cpuTime field ? If so it doesn't seem like we need to add this feature to virDomainGetCPUStats too ?
Right, it's possible but not really useful. Jan

Ján Tomko wrote:
On 04/30/2014 02:57 PM, Daniel P. Berrange wrote:
On Wed, Apr 30, 2014 at 02:51:18PM +0200, Ján Tomko wrote:
The apps should check how many stat parameters are supported for a given pCPU by calling virDomainGetCPUStats with nparams=0 and/or check the actual number of stats returned.
Could we return valid results for total CPU stats (start_cpu = -1) and just not return any for all other start_cpu values?
Wouldn't that just be duplicating what we already provide via the virDomainGetInfo cpuTime field ? If so it doesn't seem like we need to add this feature to virDomainGetCPUStats too ?
Right, it's possible but not really useful.
OK, I'll abandon this feature until I figure out how to gather this stat for specific host-cpu. Also, bhyve driver currently doesn't set cpuTime in virDomainGetInfo(), so I'll reuse my current code for overall cpu stats to populate this field. Thanks, Roman Bogorodskiy
participants (3)
-
Daniel P. Berrange
-
Ján Tomko
-
Roman Bogorodskiy