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

[Pavel Hrdina <phrdina@xxxxxxxxxx>]

On 09/22/2014 05:54 PM, John Ferlan wrote:
> 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@xxxxxxxxxx>
> > ---
> >   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".

Will fix that, thanks.

> > +    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.

They are not the same. In the eventNparams there is current number of 
typedParameter stored in eventParams and the eventMaxNparams contains 
the allocated size of the eventParams and they definitely could be 
different. The reason is that we are using VIR_RESIZE_N instead of 
VIR_EXPAND_N and it reallocate the eventParams geometrically to speedup 
things.

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

I'll add the definitions into the libvirt.h and send another version, 
thanks.

Pavel

> 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;

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list