[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Rename tunable event constants



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 redhat com>
> >---
> >  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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]