On Tue, Nov 17, 2015 at 16:00:46 +0800, Qiaowei Ren wrote:
> This patch implement the internal driver API for perf event into
> qemu driver.
>
> In addition, this patch extend virDomainListGetStats API to get
> the statistics for perf event. To do so, we add a
> 'VIR_DOMAIN_STATS_PERF' enum to causes reporting of all previously
> enabled perf events.
>
> Signed-off-by: Qiaowei Ren <qiaowei.ren@xxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 1 +
> src/qemu/qemu_domain.h | 3 +
> src/qemu/qemu_driver.c | 181 +++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_process.c | 6 ++
> 4 files changed, 191 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 69965e6..5e74b29 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1771,6 +1771,7 @@ typedef enum {
> VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */
> 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 */
> } virDomainStatsTypes;
>
> typedef enum {
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 4be998c..0348d4a 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -26,6 +26,7 @@
>
> # include "virthread.h"
> # include "vircgroup.h"
> +# include "virperf.h"
> # include "domain_addr.h"
> # include "domain_conf.h"
> # include "snapshot_conf.h"
> @@ -190,6 +191,8 @@ struct _qemuDomainObjPrivate {
>
> virCgroupPtr cgroup;
>
> + virPerfPtr perf;
> +
> virCond unplugFinished; /* signals that unpluggingDevice was unplugged */
> const char *unpluggingDevice; /* alias of the device that is being unplugged */
> char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 92a9961..5ad4d79 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -98,6 +98,7 @@
> #include "virhostdev.h"
> #include "domain_capabilities.h"
> #include "vircgroup.h"
> +#include "virperf.h"
> #include "virnuma.h"
> #include "dirname.h"
> #include "network/bridge_driver.h"
> @@ -113,6 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver");
>
> #define QEMU_NB_NUMA_PARAM 2
>
> +#define QEMU_NB_PERF_PARAM VIR_PERF_EVENT_LAST
> +
VIR_PERF_EVENT_LAST is good enough, no need to define another symbol for
it.
> #define QEMU_SCHED_MIN_PERIOD 1000LL
> #define QEMU_SCHED_MAX_PERIOD 1000000LL
> #define QEMU_SCHED_MIN_QUOTA 1000LL
> @@ -10244,6 +10247,106 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
> }
>
> static int
> +qemuDomainSetPerfEvents(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams)
> +{
> + size_t i;
> + virDomainObjPtr vm = NULL;
> + qemuDomainObjPrivatePtr priv;
> + int ret = -1;
> + bool enabled;
> +
> + if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_PERF_CMT,
> + VIR_TYPED_PARAM_BOOLEAN,
> + NULL) < 0)
> + return -1;
Use virTypedParamsCheck and define the data for it in virperf.h so that
you don't need to change the code here if new event type is added.
> +
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + return -1;
> +
> + priv = vm->privateData;
> +
> + if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < nparams; i++) {
> + virTypedParameterPtr param = ¶ms[i];
> + enabled = params[i].value.b;
> +
> + if (STREQ(param->field, VIR_DOMAIN_PERF_CMT)) {
Just use the appropriate FromString to convert the param field into the
perf type enum and call virPerfEvent{Disable,Enable} depending on the
value. Thus no change will be needed here if you add more perf events.
> + if (!enabled && virPerfEventDisable(VIR_PERF_EVENT_CMT, priv->perf)) {
> + virReportError(VIR_ERR_NO_SUPPORT,
> + _("can't disable perf event: %s"),
> + VIR_DOMAIN_PERF_CMT);
> + goto cleanup;
> + }
> + if (enabled && virPerfCmtEnable(vm->pid, priv->perf)) {
> + virReportError(VIR_ERR_NO_SUPPORT,
> + _("can't enable perf event: %s"),
> + VIR_DOMAIN_PERF_CMT);
> + goto cleanup;
> + }
Both virPerfEventDisable and virPerfEventEnable should have reported the
error, no need to mask it with another one.
> + }
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int
> +qemuDomainGetPerfEvents(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int * nparams)
> +{
> + size_t i;
> + virDomainObjPtr vm = NULL;
> + qemuDomainObjPrivatePtr priv;
> + int ret = -1;
> +
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + goto cleanup;
> +
> + priv = vm->privateData;
> +
> + if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if ((*nparams) == 0) {
> + *nparams = QEMU_NB_PERF_PARAM;
> + ret = 0;
> + goto cleanup;
> + }
> +
> + for (i = 0; i < QEMU_NB_PERF_PARAM && i < *nparams; i++) {
> + virMemoryParameterPtr param = ¶ms[i];
Comments similar to SetPerfEvents apply here as well.
> +
> + switch (i) {
> + case VIR_PERF_EVENT_CMT:
> + if (virTypedParameterAssign(param, VIR_DOMAIN_PERF_CMT,
> + VIR_TYPED_PARAM_BOOLEAN,
> + priv->perf->events[VIR_PERF_EVENT_CMT].enabled) < 0)
This should rather you an access function.
> + goto cleanup;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (*nparams > QEMU_NB_PERF_PARAM)
> + *nparams = QEMU_NB_PERF_PARAM;
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int
> qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
> unsigned long long period, long long quota)
> {
> @@ -19419,6 +19522,81 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>
> #undef QEMU_ADD_COUNT_PARAM
>
> +static int
> +qemuDomainGetStatsPerfCmt(virPerfEventPtr eventPtr,
> + virDomainStatsRecordPtr record,
> + int *maxparams)
> +{
> + FILE *fd;
> + unsigned long long cache = 0;
> + int scaling_factor = 0;
> +
> + if (eventPtr->fd <= 0 || !eventPtr->enabled)
> + return -1;
> +
> + if (read(eventPtr->fd, &cache, sizeof(uint64_t)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to read cache data"));
> + return -1;
> + }
> +
> + fd = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");
Will this value change in time? If not, we could just read it and store
within virPerfEvent when enabling it. It may be necessary to turn
virPerfEvent into union to do that.
> + if (!fd) {
> + virReportSystemError(errno, "%s",
> + _("Unable to open CMT scale file"));
> + return -1;
> + }
> + if (fscanf(fd, "%d", &scaling_factor) != 1) {
> + virReportSystemError(errno, "%s",
> + _("Unable to read CMT scale file"));
> + VIR_FORCE_FCLOSE(fd);
> + return -1;
> + }
> + VIR_FORCE_FCLOSE(fd);
>
Anyway, it looks like this would deserve to be moved to virperf.c (and
use cleanup lables).
> + cache *= scaling_factor;
> +
> + if (virTypedParamsAddULLong(&record->params,
> + &record->nparams,
> + maxparams,
> + "perf.cache",
> + cache) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams,
> + unsigned int privflags ATTRIBUTE_UNUSED)
> +{
> + size_t i;
> + qemuDomainObjPrivatePtr priv = dom->privateData;
> + int ret = -1;
> +
> + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
> + virPerfEventPtr event = virPerfEventOfType(priv->perf, i);
> + if (!event->enabled)
> + continue;
> +
> + switch (i) {
Typecast 'i' to the perf event type...
> + case VIR_PERF_EVENT_CMT:
> + if (qemuDomainGetStatsPerfCmt(event, record, maxparams))
> + goto cleanup;
> + break;
> + default:
> + break;
and remove this default branch so that the compiler reminds you to
change this code in case new perf event is added.
> + }
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> typedef int
> (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver,
> virDomainObjPtr dom,
> @@ -19439,6 +19617,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
> { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false },
> { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false },
> { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true },
> + { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false },
> { NULL, 0, false }
> };
>
> @@ -20227,6 +20406,8 @@ static virHypervisorDriver qemuHypervisorDriver = {
> .domainMigrateFinish3 = qemuDomainMigrateFinish3, /* 0.9.2 */
> .domainMigrateConfirm3 = qemuDomainMigrateConfirm3, /* 0.9.2 */
> .domainSendKey = qemuDomainSendKey, /* 0.9.4 */
> + .domainGetPerfEvents = qemuDomainGetPerfEvents, /* 1.2.23 */
> + .domainSetPerfEvents = qemuDomainSetPerfEvents, /* 1.2.23 */
> .domainBlockJobAbort = qemuDomainBlockJobAbort, /* 0.9.4 */
> .domainGetBlockJobInfo = qemuDomainGetBlockJobInfo, /* 0.9.4 */
> .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 4545f77..2d81069 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4680,6 +4680,10 @@ int qemuProcessStart(virConnectPtr conn,
> if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0)
> goto error;
>
> + VIR_DEBUG("Initializing perf event");
> + if (virPerfInitialize(&(priv->perf)) < 0)
> + goto cleanup;
This is why virPerfNew should not return a failure in case perf events
are not supported.
> +
> /* This must be done after cgroup placement to avoid resetting CPU
> * affinity */
> if (!vm->def->cputune.emulatorpin &&
> @@ -5187,6 +5191,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> }
> virCgroupFree(&priv->cgroup);
>
> + virPerfFree(&(priv->perf));
> +
> qemuProcessRemoveDomainStatus(driver, vm);
>
> /* Remove VNC and Spice ports from port reservation bitmap, but only if
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list