On Thu, Sep 25, 2014 at 07:16:39PM +0200, Pavel Hrdina wrote:
> On 09/25/2014 06:51 PM, Daniel P. Berrange wrote:
> >For the new VIR_DOMAIN_EVENT_ID_TUNABLE event we have a bunch of
> >constants added
> >
> > VIR_DOMAIN_EVENT_CPUTUNE_<blah>
> > VIR_DOMAIN_EVENT_BLKDEVTUNE_<blah>
> >
> >This naming convention is bad for two reasons
> >
> > - There is no common prefix unique for the events to both
> > relate them, and distinguish them from other event
> > constants
> >
> > - The values associated with the constants were chosen
> > to match the names used with virConnectGetAllDomainStats
> > so having EVENT in the constant name is not applicable in
> > that respect
> >
> >This patch proposes renaming the constants to
> >
> > VIR_DOMAIN_TUNABLE_CPU_<blah>
> > VIR_DOMAIN_TUNABLE_BLKDEV_<blah>
> >
> >ie, given them a common VIR_DOMAIN_TUNABLE prefix.
> >
> >Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> >---
> > include/libvirt/libvirt.h.in | 56 ++++++++++++++++++++++----------------------
> > src/qemu/qemu_cgroup.c | 2 +-
> > src/qemu/qemu_driver.c | 28 +++++++++++-----------
> > 3 files changed, 43 insertions(+), 43 deletions(-)
> >
> >diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> >index 93c9a48..2ff5204 100644
> >--- a/include/libvirt/libvirt.h.in
> >+++ b/include/libvirt/libvirt.h.in
> >@@ -5204,119 +5204,119 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
> > void *opaque);
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_VCPUPIN:
> >+ * VIR_DOMAIN_TUNABLE_CPU_VCPUPIN:
> > *
> > * Macro represents formatted pinning for one vcpu specified by id which is
> > * appended to the parameter name, for example "cputune.vcpupin1",
> > * as VIR_TYPED_PARAM_STRING.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_VCPUPIN "cputune.vcpupin%u"
> >+#define VIR_DOMAIN_TUNABLE_CPU_VCPUPIN "cputune.vcpupin%u"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_EMULATORIN:
> >+ * VIR_DOMAIN_TUNABLE_CPU_EMULATORIN:
> > *
> > * Macro represents formatted pinning for emulator process,
> > * as VIR_TYPED_PARAM_STRING.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_EMULATORIN "cputune.emulatorpin"
> >+#define VIR_DOMAIN_TUNABLE_CPU_EMULATORIN "cputune.emulatorpin"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_CPU_SHARES:
> >+ * VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES:
> > *
> > * Macro represents proportional weight of the scheduler used on the
> > * host cpu, when using the posix scheduler, as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_CPU_SHARES "cputune.cpu_shares"
> >+#define VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES "cputune.cpu_shares"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_VCPU_PERIOD:
> >+ * VIR_DOMAIN_TUNABLE_CPU_VCPU_PERIOD:
> > *
> > * Macro represents the enforcement period for a quota, in microseconds,
> > * for vcpus only, when using the posix scheduler, as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_VCPU_PERIOD "cputune.vcpu_period"
> >+#define VIR_DOMAIN_TUNABLE_CPU_VCPU_PERIOD "cputune.vcpu_period"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_VCPU_QUOTA:
> >+ * VIR_DOMAIN_TUNABLE_CPU_VCPU_QUOTA:
> > *
> > * Macro represents the maximum bandwidth to be used within a period for
> > * vcpus only, when using the posix scheduler, as VIR_TYPED_PARAM_LLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_VCPU_QUOTA "cputune.vcpu_quota"
> >+#define VIR_DOMAIN_TUNABLE_CPU_VCPU_QUOTA "cputune.vcpu_quota"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_PERIOD:
> >+ * VIR_DOMAIN_TUNABLE_CPU_EMULATOR_PERIOD:
> > *
> > * Macro represents the enforcement period for a quota in microseconds,
> > * when using the posix scheduler, for all emulator activity not tied to
> > * vcpus, as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_PERIOD "cputune.emulator_period"
> >+#define VIR_DOMAIN_TUNABLE_CPU_EMULATOR_PERIOD "cputune.emulator_period"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_QUOTA:
> >+ * VIR_DOMAIN_TUNABLE_CPU_EMULATOR_QUOTA:
> > *
> > * Macro represents the maximum bandwidth to be used within a period for
> > * all emulator activity not tied to vcpus, when using the posix scheduler,
> > * as an VIR_TYPED_PARAM_LLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_QUOTA "cputune.emulator_quota"
> >+#define VIR_DOMAIN_TUNABLE_CPU_EMULATOR_QUOTA "cputune.emulator_quota"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_DISK:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_DISK:
> > *
> > * Macro represents the name of guest disk for which the values are updated,
> > * as VIR_TYPED_PARAM_STRING.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_DISK "blkdeviotune.disk"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_DISK "blkdeviotune.disk"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_BYTES_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC:
> > *
> > * Marco represents the total throughput limit in bytes per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_BYTES_SEC "blkdeviotune.total_bytes_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC "blkdeviotune.total_bytes_sec"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_BYTES_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC:
> > *
> > * Marco represents the read throughput limit in bytes per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_BYTES_SEC "blkdeviotune.read_bytes_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC "blkdeviotune.read_bytes_sec"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_BYTES_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC:
> > *
> > * Macro represents the write throughput limit in bytes per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_BYTES_SEC "blkdeviotune.write_bytes_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC "blkdeviotune.write_bytes_sec"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_IOPS_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC:
> > *
> > * Macro represents the total I/O operations per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_IOPS_SEC "blkdeviotune.total_iops_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC "blkdeviotune.total_iops_sec"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_IOPS_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC:
> > *
> > * Macro represents the read I/O operations per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_IOPS_SEC "blkdeviotune.read_iops_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC "blkdeviotune.read_iops_sec"
> >
> > /**
> >- * VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_IOPS_SEC:
> >+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC:
> > *
> > * Macro represents the write I/O operations per second,
> > * as VIR_TYPED_PARAM_ULLONG.
> > */
> >-#define VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_IOPS_SEC "blkdeviotune.write_iops_sec"
> >+#define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC "blkdeviotune.write_iops_sec"
> >
> > /**
> > * virConnectDomainEventTunableCallback:
> >diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> >index 300946a..8819943 100644
> >--- a/src/qemu/qemu_cgroup.c
> >+++ b/src/qemu/qemu_cgroup.c
> >@@ -703,7 +703,7 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
> > vm->def->cputune.shares = val;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_CPU_SHARES,
> >+ VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES,
> > val) < 0)
> > return -1;
> >
> >diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >index 117138a..6e792a6 100644
> >--- a/src/qemu/qemu_driver.c
> >+++ b/src/qemu/qemu_driver.c
> >@@ -4653,7 +4653,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> > goto cleanup;
> >
> > if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH,
> >- VIR_DOMAIN_EVENT_CPUTUNE_VCPUPIN, vcpu) < 0) {
> >+ VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) {
> > goto cleanup;
> > }
> >
> >@@ -4940,7 +4940,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
> > str = virBitmapFormat(pcpumap);
> > if (virTypedParamsAddString(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_EMULATORIN,
> >+ VIR_DOMAIN_TUNABLE_CPU_EMULATORIN,
> > str) < 0)
> > goto cleanup;
> >
> >@@ -9318,7 +9318,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxNparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_CPU_SHARES,
> >+ VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES,
> > val) < 0)
> > goto cleanup;
> > }
> >@@ -9341,7 +9341,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxNparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_VCPU_PERIOD,
> >+ VIR_DOMAIN_TUNABLE_CPU_VCPU_PERIOD,
> > value_ul) < 0)
> > goto cleanup;
> > }
> >@@ -9361,7 +9361,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >
> > if (virTypedParamsAddLLong(&eventParams, &eventNparams,
> > &eventMaxNparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_VCPU_QUOTA,
> >+ VIR_DOMAIN_TUNABLE_CPU_VCPU_QUOTA,
> > value_l) < 0)
> > goto cleanup;
> > }
> >@@ -9382,7 +9382,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxNparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_PERIOD,
> >+ VIR_DOMAIN_TUNABLE_CPU_EMULATOR_PERIOD,
> > value_ul) < 0)
> > goto cleanup;
> > }
> >@@ -9403,7 +9403,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >
> > if (virTypedParamsAddLLong(&eventParams, &eventNparams,
> > &eventMaxNparams,
> >- VIR_DOMAIN_EVENT_CPUTUNE_EMULATOR_QUOTA,
> >+ VIR_DOMAIN_TUNABLE_CPU_EMULATOR_QUOTA,
> > value_l) < 0)
> > goto cleanup;
> > }
> >@@ -16321,7 +16321,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > goto endjob;
> >
> > if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_DISK, disk) < 0)
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_DISK, disk) < 0)
> > goto endjob;
> >
> > for (i = 0; i < nparams; i++) {
> >@@ -16339,7 +16339,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_bytes = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_BYTES_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > } else if (STREQ(param->field,
> >@@ -16348,7 +16348,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_bytes = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_BYTES_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > } else if (STREQ(param->field,
> >@@ -16357,7 +16357,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_bytes = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_BYTES_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > } else if (STREQ(param->field,
> >@@ -16366,7 +16366,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_iops = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_TOTAL_IOPS_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > } else if (STREQ(param->field,
> >@@ -16375,7 +16375,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_iops = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_READ_IOPS_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > } else if (STREQ(param->field,
> >@@ -16384,7 +16384,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> > set_iops = true;
> > if (virTypedParamsAddULLong(&eventParams, &eventNparams,
> > &eventMaxparams,
> >- VIR_DOMAIN_EVENT_BLKDEVIOTUNE_WRITE_IOPS_SEC,
> >+ VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC,
> > param->value.ul) < 0)
> > goto endjob;
> > }
> >
>
>
> I would probably leave the "IO" in the macro name:
>
> VIR_DOMAIN_TUNABLE_BLKDEVIO_DISK
>
> to make it clear that it's for tuning I/O values.
What if it isn't about I/O values though - eg we could be reporting qcow2
disk high watermark, which isn't an I/O tuning thing, rather a host
allocation thing.
> ACK whether you change the BLKDEV to BLKDEVIO or not.
I think I'll stick with just BLKDEV unless someone has other reasons
to disagree ?
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list