Re: [libvirt] [PATCH v4 4/4] cputune_event: queue the event for cputune updates

On 09/18/2014 04:39 AM, Pavel Hrdina wrote: > Now we have universal tunable event so we can use it for reporting > changes to user. The cputune values will be prefixed with "cputune" to > distinguish it from other tunable events. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/qemu/qemu_cgroup.c | 18 +++++++++++- > src/qemu/qemu_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 93 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 9d39370..27dcba3 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -34,6 +34,7 @@ > #include "virscsi.h" > #include "virstring.h" > #include "virfile.h" > +#include "virtypedparam.h" > > #define VIR_FROM_THIS VIR_FROM_QEMU > > @@ -676,6 +677,10 @@ static int > qemuSetupCpuCgroup(virDomainObjPtr vm) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > + virObjectEventPtr event = NULL; > + virTypedParameterPtr eventParams; Consistency with other defs "eventParams = NULL".

> + int eventNparams = 0; > + int eventMaxparams = 0; > > if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { > if (vm->def->cputune.sharesSpecified) { > @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) > > if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) > return -1; > - vm->def->cputune.shares = val; > + if (vm->def->cputune.shares != val) { > + vm->def->cputune.shares = val; > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, > + &eventMaxparams, "cputune.shares", Perhaps this name space ('cputune.*') should be what goes into libvirt.h (mentioned in the v2 3/5 review). That is define and use #define VIR_DOMAIN_EVENT_CPUTUNE_SHARES "cputune.shares" > + val) < 0) > + return -1; > + > + event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams); > + } > + > + if (event) > + qemuDomainEventQueue(vm->privateData, event); > } > > return 0; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 5fd89a3..22c6e55 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4538,6 +4538,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, > virBitmapPtr pcpumap = NULL; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > + virObjectEventPtr event = NULL; > + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; > + char *str = NULL; > + virTypedParameterPtr eventParams = NULL; > + int eventNparams = 0; > + int eventMaxparams = 0; > > virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > VIR_DOMAIN_AFFECT_CONFIG, -1); > @@ -4645,6 +4651,18 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, > > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) > goto cleanup; > + > + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, > + "cputune.vcpupin%d", vcpu) < 0) { #define VIR_DOMAIN_EVENT_CPUTUNE_VCPUPIN "cputune.vcpupin%u" BTW: vcpu is an unsigned... Somehow have to comment that the event will *start with* this string with the postfix being the vCPU number as defined by the guest. > + goto cleanup; > + } > + > + str = virBitmapFormat(pcpumap); > + if (virTypedParamsAddString(&eventParams, &eventNparams, > + &eventMaxparams, paramField, str) < 0) > + goto cleanup; > + > + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > @@ -4680,6 +4698,9 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, > virCgroupFree(&cgroup_vcpu); > if (vm) > virObjectUnlock(vm); > + if (event) > + qemuDomainEventQueue(driver, event); > + VIR_FREE(str); > virBitmapFree(pcpumap); > virObjectUnref(caps); > virObjectUnref(cfg); > @@ -4804,6 +4825,12 @@ qemuDomainPinEmulator(virDomainPtr dom, > virBitmapPtr pcpumap = NULL; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > + virObjectEventPtr event = NULL; > + char * str = NULL; > + virTypedParameterPtr eventParams = NULL; > + int eventNparams = 0; > + int eventMaxparams = 0; > + > > virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > VIR_DOMAIN_AFFECT_CONFIG, -1); > @@ -4909,6 +4936,13 @@ qemuDomainPinEmulator(virDomainPtr dom, > > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) > goto cleanup; > + > + str = virBitmapFormat(pcpumap); > + if (virTypedParamsAddString(&eventParams, &eventNparams, > + &eventMaxparams, "cputune.emulatorpin", str) < 0) #define VIR_DOMAIN_EVENT_CPUTUNE_EMULATORPIN "cputune.emulatorpin" > + goto cleanup; > + > + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > @@ -4938,6 +4972,9 @@ qemuDomainPinEmulator(virDomainPtr dom, > cleanup: > if (cgroup_emulator) > virCgroupFree(&cgroup_emulator); > + if (event) > + qemuDomainEventQueue(driver, event); > + VIR_FREE(str); > virBitmapFree(pcpumap); > virObjectUnref(caps); > if (vm) > @@ -9096,6 +9133,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > qemuDomainObjPrivatePtr priv; > + virObjectEventPtr event = NULL; > + virTypedParameterPtr eventParams = NULL; > + int eventNparams = 0; > + int eventMaxNparams = 0; > > virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > VIR_DOMAIN_AFFECT_CONFIG, -1); > @@ -9166,6 +9207,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > > vm->def->cputune.shares = val; > vm->def->cputune.sharesSpecified = true; > + > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, > + &eventMaxNparams, "cputune.shares", VIR_DOMAIN_EVENT_CPUTUNE_SHARES > + val) < 0) > + goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > @@ -9183,6 +9229,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > goto cleanup; > > vm->def->cputune.period = value_ul; > + > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, > + &eventMaxNparams, "cputune.period", #define VIR_DOMAIN_EVENT_CPUTUNE_VCPU_PERIOD "cputune.vcpu_period" > + value_ul) < 0) > + goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) > @@ -9197,6 +9248,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > goto cleanup; > > vm->def->cputune.quota = value_l; > + > + if (virTypedParamsAddLLong(&eventParams, &eventNparams, > + &eventMaxNparams, "cputune.quota", #define VIR_DOMAIN_EVENT_CPUTUNE_VCPU_QUOTA "cputune.vcpu_quota" > + value_l) < 0) > + goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) > @@ -9212,6 +9268,12 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > goto cleanup; > > vm->def->cputune.emulator_period = value_ul; > + > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, > + &eventMaxNparams, > + "cputune.emulator_period", #define VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_PERIOD "cputune.emulator_period" > + value_ul) < 0) > + goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) > @@ -9227,6 +9289,12 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > goto cleanup; > > vm->def->cputune.emulator_quota = value_l; > + > + if (virTypedParamsAddLLong(&eventParams, &eventNparams, > + &eventMaxNparams, > + "cputune.emulator_quota", #define VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_QUOTA "cputune.emulator_quota" > + value_l) < 0) > + goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) > @@ -9237,6 +9305,12 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) > goto cleanup; > > + if (eventNparams) { > + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); FWIW: This is the code that got me to thinking in 2/4 about the usage of eventMaxNparams vs. just eventNparams. They I believe are the same, but can we "foresee" any reason they could/would differ.

In any case, I think with the usage of libvirt.h #define's this is ACK-able.

John > + eventNparams = 0; > + if (event) > + qemuDomainEventQueue(driver, event); > + } > > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > rc = virDomainSaveConfig(cfg->configDir, vmdef); > @@ -9253,6 +9327,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, > virDomainDefFree(vmdef); > if (vm) > virObjectUnlock(vm); > + if (eventNparams) > + virTypedParamsFree(eventParams, eventNparams); > virObjectUnref(caps); > virObjectUnref(cfg); > return ret; >