[libvirt] [PATCH 0/6] Couple of perf events APIs fixes

I really like to see these in before release and thus APIs get written in the stone. One note though regarding the last patch: while writing it I've noticed couple of other getter APIs don't grab any job at all. I think they should grab _QUERY job though. So maybe those will need some fixing too. Michal Privoznik (6): remoteDomainGetPerfEvents: Re-indent virDomain{Get,Set}PerfEvents: Add @flags argument virsh: Prefer VIRSH_COMMON_OPT_DOMAIN_FULL over full enumeration virsh: Make perf accept event list separated by commas virDomain{Get,Set}PerfEvents: support --config --live --current virDomain{Get,Set}PerfEvents: Grab job daemon/remote.c | 2 +- include/libvirt/libvirt-domain.h | 6 ++- src/driver-hypervisor.h | 6 ++- src/libvirt-domain.c | 22 +++++---- src/qemu/qemu_driver.c | 99 +++++++++++++++++++++++++++++----------- src/remote/remote_driver.c | 12 +++-- src/remote/remote_protocol.x | 2 + tools/virsh-domain.c | 27 +++++++---- tools/virsh.pod | 25 ++++++---- 9 files changed, 141 insertions(+), 60 deletions(-) -- 2.7.3

There are few lines off the indentation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b3c7b9c..254e0e9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1837,10 +1837,10 @@ remoteDomainGetPerfEvents(virDomainPtr domain, goto done; if (virTypedParamsDeserialize((virTypedParameterRemotePtr) ret.params.params_val, - ret.params.params_len, - REMOTE_DOMAIN_PERF_EVENTS_MAX, - params, - nparams) < 0) + ret.params.params_len, + REMOTE_DOMAIN_PERF_EVENTS_MAX, + params, + nparams) < 0) goto cleanup; rv = 0; -- 2.7.3

On Thu, Mar 31, 2016 at 07:28:56 +0200, Michal Privoznik wrote:
There are few lines off the indentation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK

I've noticed that these APIs are missing @flags argument. Even though we don't have a use for them, it's our policy that every new API must have @flags. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/remote.c | 2 +- include/libvirt/libvirt-domain.h | 6 ++++-- src/driver-hypervisor.h | 6 ++++-- src/libvirt-domain.c | 22 ++++++++++++++-------- src/qemu/qemu_driver.c | 11 ++++++++--- src/remote/remote_driver.c | 4 +++- src/remote/remote_protocol.x | 2 ++ tools/virsh-domain.c | 5 +++-- 8 files changed, 39 insertions(+), 19 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index bbe22b0..9db93ff 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2694,7 +2694,7 @@ remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED, if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; - if (virDomainGetPerfEvents(dom, ¶ms, &nparams) < 0) + if (virDomainGetPerfEvents(dom, ¶ms, &nparams, args->flags) < 0) goto cleanup; if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) { diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 552a40b..728b6eb 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1848,10 +1848,12 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, - int *nparams); + int *nparams, + unsigned int flags); int virDomainSetPerfEvents(virDomainPtr dom, virTypedParameterPtr params, - int nparams); + int nparams, + unsigned int flags); /* * BlockJob API diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index d0e7298..d11ff7f 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -964,12 +964,14 @@ typedef int typedef int (*virDrvDomainGetPerfEvents)(virDomainPtr dom, virTypedParameterPtr *params, - int *nparams); + int *nparams, + unsigned int flags); typedef int (*virDrvDomainSetPerfEvents)(virDomainPtr dom, virTypedParameterPtr params, - int nparams); + int nparams, + unsigned int flags); typedef int (*virDrvDomainBlockJobAbort)(virDomainPtr dom, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 42031bc..3e144b6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -9695,6 +9695,7 @@ virDomainOpenChannel(virDomainPtr dom, * @domain: a domain object * @params: where to store perf events setting * @nparams: number of items in @params + * @flags: extra flags; not used yet, so callers should always pass 0 * * Get all perf events setting. Possible fields returned in @params are * defined by VIR_DOMAIN_PERF_* macros and new fields will likely be @@ -9704,12 +9705,13 @@ virDomainOpenChannel(virDomainPtr dom, */ int virDomainGetPerfEvents(virDomainPtr domain, virTypedParameterPtr *params, - int *nparams) + int *nparams, + unsigned int flags) { virConnectPtr conn; - VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p", - params, nparams); + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p flags=%x", + params, nparams, flags); virResetLastError(); @@ -9721,7 +9723,8 @@ int virDomainGetPerfEvents(virDomainPtr domain, if (conn->driver->domainGetPerfEvents) { int ret; - ret = conn->driver->domainGetPerfEvents(domain, params, nparams); + ret = conn->driver->domainGetPerfEvents(domain, params, + nparams, flags); if (ret < 0) goto error; return ret; @@ -9740,6 +9743,7 @@ int virDomainGetPerfEvents(virDomainPtr domain, * @params: pointer to perf events parameter object * @nparams: number of perf event parameters (this value can be the same * less than the number of parameters supported) + * @flags: extra flags; not used yet, so callers should always pass 0 * * Enable or disable the particular list of perf events you care about. * @@ -9747,12 +9751,13 @@ int virDomainGetPerfEvents(virDomainPtr domain, */ int virDomainSetPerfEvents(virDomainPtr domain, virTypedParameterPtr params, - int nparams) + int nparams, + unsigned int flags) { virConnectPtr conn; - VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d", - params, nparams); + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d flags=%x", + params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); virResetLastError(); @@ -9769,7 +9774,8 @@ int virDomainSetPerfEvents(virDomainPtr domain, if (conn->driver->domainSetPerfEvents) { int ret; - ret = conn->driver->domainSetPerfEvents(domain, params, nparams); + ret = conn->driver->domainSetPerfEvents(domain, params, + nparams, flags); if (ret < 0) goto error; return ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a0a33a4..cbd520b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10040,7 +10040,8 @@ qemuSetGlobalBWLive(virCgroupPtr cgroup, unsigned long long period, static int qemuDomainSetPerfEvents(virDomainPtr dom, virTypedParameterPtr params, - int nparams) + int nparams, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; size_t i; @@ -10049,11 +10050,12 @@ qemuDomainSetPerfEvents(virDomainPtr dom, qemuDomainObjPrivatePtr priv; virDomainDefPtr def; virDomainDefPtr persistentDef; - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; int ret = -1; virPerfEventType type; bool enabled; + virCheckFlags(0, -1); + if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0) return -1; @@ -10107,7 +10109,8 @@ qemuDomainSetPerfEvents(virDomainPtr dom, static int qemuDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, - int *nparams) + int *nparams, + unsigned int flags) { size_t i; virDomainObjPtr vm = NULL; @@ -10117,6 +10120,8 @@ qemuDomainGetPerfEvents(virDomainPtr dom, int maxpar = 0; int npar = 0; + virCheckFlags(0, -1); + if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 254e0e9..b03c9ca 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1819,7 +1819,8 @@ remoteDomainGetNumaParameters(virDomainPtr domain, static int remoteDomainGetPerfEvents(virDomainPtr domain, virTypedParameterPtr *params, - int *nparams) + int *nparams, + unsigned int flags) { int rv = -1; remote_domain_get_perf_events_args args; @@ -1829,6 +1830,7 @@ remoteDomainGetPerfEvents(virDomainPtr domain, remoteDriverLock(priv); make_nonnull_domain(&args.dom, domain); + args.flags = flags; memset(&ret, 0, sizeof(ret)); if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_PERF_EVENTS, diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index cb19bc0..8bda792 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -634,10 +634,12 @@ struct remote_domain_get_numa_parameters_ret { struct remote_domain_set_perf_events_args { remote_nonnull_domain dom; remote_typed_param params<REMOTE_DOMAIN_PERF_EVENTS_MAX>; + unsigned int flags; }; struct remote_domain_get_perf_events_args { remote_nonnull_domain dom; + unsigned int flags; }; struct remote_domain_get_perf_events_ret { diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cda442d..a105268 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8610,6 +8610,7 @@ cmdPerf(vshControl *ctl, const vshCmd *cmd) virTypedParameterPtr params = NULL; bool ret = false; const char *enable = NULL, *disable = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -8627,7 +8628,7 @@ cmdPerf(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (nparams == 0) { - if (virDomainGetPerfEvents(dom, ¶ms, &nparams) != 0) { + if (virDomainGetPerfEvents(dom, ¶ms, &nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to get perf events")); goto cleanup; } @@ -8640,7 +8641,7 @@ cmdPerf(vshControl *ctl, const vshCmd *cmd) } } } else { - if (virDomainSetPerfEvents(dom, params, nparams) != 0) + if (virDomainSetPerfEvents(dom, params, nparams, flags) != 0) goto error; } -- 2.7.3

On Thu, Mar 31, 2016 at 07:28:57 +0200, Michal Privoznik wrote:
I've noticed that these APIs are missing @flags argument. Even though we don't have a use for them, it's our policy that every new API must have @flags.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/remote.c | 2 +- include/libvirt/libvirt-domain.h | 6 ++++-- src/driver-hypervisor.h | 6 ++++-- src/libvirt-domain.c | 22 ++++++++++++++-------- src/qemu/qemu_driver.c | 11 ++++++++--- src/remote/remote_driver.c | 4 +++- src/remote/remote_protocol.x | 2 ++ tools/virsh-domain.c | 5 +++-- 8 files changed, 39 insertions(+), 19 deletions(-)
struct remote_domain_get_perf_events_ret { diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cda442d..a105268 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8610,6 +8610,7 @@ cmdPerf(vshControl *ctl, const vshCmd *cmd) virTypedParameterPtr params = NULL; bool ret = false; const char *enable = NULL, *disable = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
There is no such flag option currently, you should pass 0. (I know that VIR_DOMAIN_AFFECT_CURRENT is actually equal to 0, but in the context of this patch you are stating that there are no supported flags.)
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false;
ACK, disregard my above statement. Fixing the 'unpleasant' code is more important at this point. Peter

We have a macro that does exactly what is done via full enumeration. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a105268..a09cdec 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8551,11 +8551,7 @@ static const vshCmdInfo info_perf[] = { }; static const vshCmdOptDef opts_perf[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN_FULL, {.name = "enable", .type = VSH_OT_STRING, .help = N_("perf events which will be enabled") -- 2.7.3

On Thu, Mar 31, 2016 at 07:28:58 +0200, Michal Privoznik wrote:
We have a macro that does exactly what is done via full enumeration.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
I don't prefer the macro, but ACK for consistency

Everywhere else we use a comma separated list. There's no good reason to make 'perf' command an exception. Currently, it accepts string list separated by '|'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 2 +- tools/virsh.pod | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a09cdec..2dbb890 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8575,7 +8575,7 @@ virshParseEventStr(vshControl *ctl, size_t i, ntok; int ret = -1; - if (!(tok = virStringSplitCount(event, "|", 0, &ntok))) + if (!(tok = virStringSplitCount(event, ",", 0, &ntok))) return -1; if (ntok > VIR_PERF_EVENT_LAST) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 0c02d7f..a9915b0 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2144,11 +2144,11 @@ The guaranteed minimum memory allocation for the guest. Specifying -1 as a value for these limits is interpreted as unlimited. -=item B<perf> I<domain> [I<--enable> B<eventName>] -[I<--disable> B<eventName>] +=item B<perf> I<domain> [I<--enable> B<eventSpec>] +[I<--disable> B<eventSpec>] Get the current perf events setting or enable/disable specific perf -event for a guest domain. +events for a guest domain. Perf is a performance analyzing tool in Linux, and it can instrument CPU performance counters, tracepoints, kprobes, and uprobes (dynamic @@ -2158,11 +2158,12 @@ pure kernel counters, in this case they are called software events, including context-switches, minor-faults, etc.. Now dozens of events from different sources can be supported by perf. -Currently only QEMU/KVM supports I<--enable> and I<--disable>. -B<eventName> is a string listing one or more events, in the format -of name|name|name. Only "cmt" event is supported presently. CMT is -a PQos (Platform Qos) feature to monitor the usage of cache by -applications running on the platform. +Currently only QEMU/KVM supports this command. The I<--enable> and I<--disable> +option combined with B<eventSpec> can be used to enabled or disable specific +performance event. B<eventSpec> is a string list of one or more events +separated by commas. However, just "cmt" event is supported presently. CMT is a +PQos (Platform Qos) feature to monitor the usage of cache by applications +running on the platform. =item B<blkiotune> I<domain> [I<--weight> B<weight>] [I<--device-weights> B<device-weights>] -- 2.7.3

On Thu, Mar 31, 2016 at 07:28:59 +0200, Michal Privoznik wrote:
Everywhere else we use a comma separated list. There's no good reason to make 'perf' command an exception. Currently, it accepts string list separated by '|'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 2 +- tools/virsh.pod | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a09cdec..2dbb890 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8575,7 +8575,7 @@ virshParseEventStr(vshControl *ctl, size_t i, ntok; int ret = -1;
- if (!(tok = virStringSplitCount(event, "|", 0, &ntok))) + if (!(tok = virStringSplitCount(event, ",", 0, &ntok))) return -1;
if (ntok > VIR_PERF_EVENT_LAST) {
While at it, you could also remove this condition. It doesn't make sense with typed param apis and hinders forward compatibility. ACK with or without the condition removed.

Now that we have @flags we can support changing perf events just in active or inactive configuration regardless of the other. Previously, calling virDomainSetPerfEvents set events in both active and inactive configuration at once. Even though we allow users to set perf events that are to be enabled once domain is started up. The virDomainGetPerfEvents API was flawed too. It returned just runtime info. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 76 +++++++++++++++++++++++++++++++++++--------------- tools/virsh-domain.c | 14 ++++++++++ tools/virsh.pod | 8 ++++++ 3 files changed, 75 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbd520b..56120ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10054,7 +10054,8 @@ qemuDomainSetPerfEvents(virDomainPtr dom, virPerfEventType type; bool enabled; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0) return -1; @@ -10071,31 +10072,37 @@ qemuDomainSetPerfEvents(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto cleanup; - for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = ¶ms[i]; - enabled = params->value.b; - type = virPerfEventTypeFromString(param->field); + if (def) { + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + enabled = params->value.b; + type = virPerfEventTypeFromString(param->field); - if (!enabled && virPerfEventDisable(priv->perf, type)) - goto cleanup; - if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) - goto cleanup; + if (!enabled && virPerfEventDisable(priv->perf, type)) + goto cleanup; + if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) + goto cleanup; - if (def) { def->perf->events[type] = enabled ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - goto cleanup; } - if (persistentDef) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + goto cleanup; + } + + if (persistentDef) { + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + enabled = params->value.b; + type = virPerfEventTypeFromString(param->field); + persistentDef->perf->events[type] = enabled ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - - if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0) - goto cleanup; } + + if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0) + goto cleanup; } ret = 0; @@ -10112,28 +10119,50 @@ qemuDomainGetPerfEvents(virDomainPtr dom, int *nparams, unsigned int flags) { - size_t i; + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; qemuDomainObjPrivatePtr priv; - int ret = -1; + virDomainDefPtr persistentDef; virTypedParameterPtr par = NULL; + virCapsPtr caps = NULL; int maxpar = 0; int npar = 0; + size_t i; + int ret = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + /* We don't return strings, and thus trivially support this flag. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto cleanup; + + priv = vm->privateData; + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + bool perf_enabled; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + perf_enabled = persistentDef->perf->events[i] == VIR_TRISTATE_BOOL_YES; + else + perf_enabled = virPerfEventIsEnabled(priv->perf, i); + if (virTypedParamsAddBoolean(&par, &npar, &maxpar, virPerfEventTypeToString(i), - virPerfEventIsEnabled(priv->perf, i)) < 0) { + perf_enabled) < 0) { virTypedParamsFree(par, npar); goto cleanup; } @@ -10145,6 +10174,7 @@ qemuDomainGetPerfEvents(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); + virObjectUnref(caps); return ret; } diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2dbb890..5865308 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8560,6 +8560,9 @@ static const vshCmdOptDef opts_perf[] = { .type = VSH_OT_STRING, .help = N_("perf events which will be disabled") }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -8607,6 +8610,17 @@ cmdPerf(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *enable = NULL, *disable = NULL; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index a9915b0..d2cc5b2 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2146,6 +2146,7 @@ Specifying -1 as a value for these limits is interpreted as unlimited. =item B<perf> I<domain> [I<--enable> B<eventSpec>] [I<--disable> B<eventSpec>] +[[I<--config>] [I<--live>] | [I<--current>]] Get the current perf events setting or enable/disable specific perf events for a guest domain. @@ -2165,6 +2166,13 @@ separated by commas. However, just "cmt" event is supported presently. CMT is a PQos (Platform Qos) feature to monitor the usage of cache by applications running on the platform. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. If no flag is specified, behavior is different depending +on hypervisor. + =item B<blkiotune> I<domain> [I<--weight> B<weight>] [I<--device-weights> B<device-weights>] [I<--device-read-iops-sec> B<device-read-iops-sec>] -- 2.7.3

On Thu, Mar 31, 2016 at 07:29:00 +0200, Michal Privoznik wrote:
Now that we have @flags we can support changing perf events just in active or inactive configuration regardless of the other. Previously, calling virDomainSetPerfEvents set events in both active and inactive configuration at once. Even though we allow users to set perf events that are to be enabled once domain is started up. The virDomainGetPerfEvents API was flawed too. It returned just runtime info.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 76 +++++++++++++++++++++++++++++++++++--------------- tools/virsh-domain.c | 14 ++++++++++ tools/virsh.pod | 8 ++++++ 3 files changed, 75 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbd520b..56120ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10054,7 +10054,8 @@ qemuDomainSetPerfEvents(virDomainPtr dom, virPerfEventType type; bool enabled;
- virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1);
if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0)
VIR_PERF_PARAMETERS is unfortunately not required to be kept in sync with the virPerfEvent enum ...
return -1; @@ -10071,31 +10072,37 @@ qemuDomainSetPerfEvents(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto cleanup;
- for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = ¶ms[i]; - enabled = params->value.b; - type = virPerfEventTypeFromString(param->field); + if (def) { + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + enabled = params->value.b; + type = virPerfEventTypeFromString(param->field);
- if (!enabled && virPerfEventDisable(priv->perf, type)) - goto cleanup; - if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) - goto cleanup; + if (!enabled && virPerfEventDisable(priv->perf, type)) + goto cleanup; + if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) + goto cleanup;
- if (def) { def->perf->events[type] = enabled ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - goto cleanup; }
- if (persistentDef) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + goto cleanup; + } + + if (persistentDef) { + for (i = 0; i < nparams; i++) {
You could go with one loop, that sets both the live and persistent defs as it was previously. Yoy just need to move the section that is actually enabling the perf events into 'if (def)'
+ virTypedParameterPtr param = ¶ms[i]; + enabled = params->value.b; + type = virPerfEventTypeFromString(param->field);
... thus you probably should make sure that the returned data makes sense. This can be fixed later though since it is currently in sync.
+ persistentDef->perf->events[type] = enabled ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - - if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0) - goto cleanup; } + + if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0) + goto cleanup; }
ret = 0; @@ -10112,28 +10119,50 @@ qemuDomainGetPerfEvents(virDomainPtr dom, int *nparams, unsigned int flags) { - size_t i; + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; qemuDomainObjPrivatePtr priv; - int ret = -1; + virDomainDefPtr persistentDef; virTypedParameterPtr par = NULL; + virCapsPtr caps = NULL; int maxpar = 0; int npar = 0; + size_t i; + int ret = -1;
- virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + /* We don't return strings, and thus trivially support this flag. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
I don't think we need this for APIs that were introduced after the support for string typed params.
if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup;
- priv = vm->privateData; - if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0)
Don't use this function in the qemu driver. The suggested replacement is virDomainObjGetOneDef. With that function you don't need @caps or @driver. If you need @flags tweaked for the function below, then look into that function for the helper.
+ goto cleanup; + + priv = vm->privateData; + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + bool perf_enabled; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + perf_enabled = persistentDef->perf->events[i] == VIR_TRISTATE_BOOL_YES; + else + perf_enabled = virPerfEventIsEnabled(priv->perf, i);
Sigh, why is that stored in 'priv' rather than 'def'? If we store it in @def then this if won't be required.
+ if (virTypedParamsAddBoolean(&par, &npar, &maxpar, virPerfEventTypeToString(i), - virPerfEventIsEnabled(priv->perf, i)) < 0) { + perf_enabled) < 0) { virTypedParamsFree(par, npar); goto cleanup; }
ACK iff you get rid of the usage of virDomainLiveConfigHelperMethod. Other comments are optional.

Even though we have the machine locked throughout whole APIs we are querying/modifying domain internal state. We should grab a job whilst doing that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56120ff..7fef901 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10069,9 +10069,12 @@ qemuDomainSetPerfEvents(virDomainPtr dom, if (virDomainSetPerfEventsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + if (def) { for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -10079,16 +10082,16 @@ qemuDomainSetPerfEvents(virDomainPtr dom, type = virPerfEventTypeFromString(param->field); if (!enabled && virPerfEventDisable(priv->perf, type)) - goto cleanup; + goto endjob; if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) - goto cleanup; + goto endjob; def->perf->events[type] = enabled ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - goto cleanup; + goto endjob; } if (persistentDef) { @@ -10102,11 +10105,14 @@ qemuDomainSetPerfEvents(virDomainPtr dom, } if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0) - goto cleanup; + goto endjob; } ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: virDomainObjEndAPI(&vm); virObjectUnref(cfg); @@ -10146,9 +10152,12 @@ qemuDomainGetPerfEvents(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &persistentDef) < 0) - goto cleanup; + goto endjob; priv = vm->privateData; @@ -10164,7 +10173,7 @@ qemuDomainGetPerfEvents(virDomainPtr dom, virPerfEventTypeToString(i), perf_enabled) < 0) { virTypedParamsFree(par, npar); - goto cleanup; + goto endjob; } } @@ -10172,6 +10181,9 @@ qemuDomainGetPerfEvents(virDomainPtr dom, *nparams = npar; ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: virDomainObjEndAPI(&vm); virObjectUnref(caps); -- 2.7.3

On Thu, Mar 31, 2016 at 07:29:01 +0200, Michal Privoznik wrote:
Even though we have the machine locked throughout whole APIs we are querying/modifying domain internal state. We should grab a job whilst doing that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
ACK, but you will get merge conflicts after fixing 5/6

On Thu, Mar 31, 2016 at 07:28:55 +0200, Michal Privoznik wrote:
I really like to see these in before release and thus APIs get written in the stone.
One note though regarding the last patch: while writing it I've noticed couple of other getter APIs don't grab any job at all. I think they should grab _QUERY job though. So maybe those will need some fixing too.
I've more-or-less ACKed the series, but it would be great if you could improve the documentation for the new APIs since it really doesn't tell anybody what's happening there. Additionally the docs for 'virDomainGetPerfEvents' states that @params is filled by 'VIR_DOMAIN_PERF_*' macros, but in reality the macro has a different prefix: VIR_PERF_PARAM_CMT Thanks for cleaning up the mess though. Peter
participants (2)
-
Michal Privoznik
-
Peter Krempa