[libvirt] [PATCH 00/12] Add length (duration) params for iotune throttling
https://bugzilla.redhat.com/show_bug.cgi?id=1349898 Do a little housekeeping and minor adjustments to existing code, then add the various "-length" options for the <iotune> code. John Ferlan (12): docs: Fix typo in libvirt-domain.h parameter description include: Update description for <iotune> max params tests: Add blkdeviotune-max xml2xmltest qemu: Convert from shorthand to longer throttling names qemu: Adjust how supportMaxOptions is used. include: Add new definitions for duration for bps/iops throttling caps: Add new capability for the bps/iops throttling length qemu: Add length for bps/iops throttling parameters to driver conf: Add a formatting macro for all the blkiotune values conf: Adjust the PARSE_IOTUNE macro conf: Add support for blkiotune "_length" options qemu: Add the length options to the iotune command line docs/formatdomain.html.in | 40 +++++- docs/schemas/domaincommon.rng | 38 ++++++ include/libvirt/libvirt-domain.h | 124 ++++++++++++++++-- src/conf/domain_conf.c | 142 +++++++++++---------- src/conf/domain_conf.h | 6 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 96 ++++++-------- src/qemu/qemu_driver.c | 106 ++++++++++++++- src/qemu/qemu_monitor.c | 7 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 72 ++++++----- src/qemu/qemu_monitor_json.h | 3 +- .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 17 ++- .../qemuxml2argv-blkdeviotune-max-length.args | 34 +++++ .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++ .../qemuxml2argv-blkdeviotune-max.args | 10 +- .../qemuxml2argv-blkdeviotune-max.xml | 14 +- .../qemuxml2argv-blkdeviotune.args | 5 +- tests/qemuxml2argvtest.c | 4 + .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + .../qemuxml2xmlout-blkdeviotune-max.xml | 1 + tests/qemuxml2xmltest.c | 2 + 28 files changed, 616 insertions(+), 182 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml -- 2.7.4
Change Marco to Macro Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f1c35d9..52680cf 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3695,7 +3695,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC: * - * Marco represents the total throughput limit in bytes per second, + * Macro represents the total throughput limit in bytes per second, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC "blkdeviotune.total_bytes_sec" @@ -3703,7 +3703,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC: * - * Marco represents the read throughput limit in bytes per second, + * Macro represents the read throughput limit in bytes per second, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC "blkdeviotune.read_bytes_sec" @@ -3743,7 +3743,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX: * - * Marco represents the total throughput limit in maximum bytes per second, + * Macro represents the total throughput limit in maximum bytes per second, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX "blkdeviotune.total_bytes_sec_max" @@ -3751,7 +3751,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX: * - * Marco represents the read throughput limit in maximum bytes per second, + * Macro represents the read throughput limit in maximum bytes per second, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX "blkdeviotune.read_bytes_sec_max" -- 2.7.4
The upstream qemu commit 'dce13204' changed the wording just slightly to add 'in bursts' essentially. Just following that model here. Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 52680cf..86126d0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3743,31 +3743,31 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX: * - * Macro represents the total throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the total throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX "blkdeviotune.total_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX: * - * Macro represents the read throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the read throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX "blkdeviotune.read_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX: * - * Macro represents the write throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the write throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX "blkdeviotune.write_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX: * - * Macro represents the total maximum I/O operations per second, + * Macro represents the total maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX "blkdeviotune.total_iops_sec_max" @@ -3775,7 +3775,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX: * - * Macro represents the read maximum I/O operations per second, + * Macro represents the read maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX "blkdeviotune.read_iops_sec_max" @@ -3783,7 +3783,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX: * - * Macro represents the write maximum I/O operations per second, + * Macro represents the write maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX "blkdeviotune.write_iops_sec_max" -- 2.7.4
It was missing... Also since I'm using the soft link from qemuxml2xmloutdata to the qemuxml2argvdata file, modify the output file to have the necessary <address> elements plus the mouse and keyboard. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml | 14 +++++++++++--- .../qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml | 1 + tests/qemuxml2xmltest.c | 1 + 3 files changed, 13 insertions(+), 3 deletions(-) create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml index 8b3e05c..ac4a0c0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml @@ -43,9 +43,17 @@ </iotune> <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> - <controller type='usb' index='0'/> - <controller type='ide' index='0'/> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> <controller type='pci' index='0' model='pci-root'/> - <memballoon model='virtio'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml new file mode 120000 index 0000000..183e1f1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index fb05c85..4b58986 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -607,6 +607,7 @@ mymain(void) DO_TEST("usb-redir-filter", NONE); DO_TEST("usb-redir-filter-version", NONE); DO_TEST("blkdeviotune", NONE); + DO_TEST("blkdeviotune-max", NONE); DO_TEST("controller-usb-order", NONE); DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); -- 2.7.4
We're about to add 6 new options and it appears (from testing) one cannot utilize both the shorthand (alias) and (much) longer names for the arguments. So modify the command builder to use the longer name and of course alter the test output .args to have the similarly innocuous long name. Also utilize a macro to build that name makes it so much more visually appealing and saves a few characters or potential cut-n-paste issues. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 77 +++++----------------- .../qemuxml2argv-blkdeviotune-max.args | 10 ++- .../qemuxml2argv-blkdeviotune.args | 5 +- 3 files changed, 28 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ac0fa1..20fe6fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1766,70 +1766,29 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, goto error; } - if (disk->blkdeviotune.total_bytes_sec) { - virBufferAsprintf(&opt, ",bps=%llu", - disk->blkdeviotune.total_bytes_sec); +#define IOTUNE_ADD(_field, _label) \ + if (disk->blkdeviotune._field) { \ + virBufferAsprintf(&opt, ",throttling." _label "=%llu", \ + disk->blkdeviotune._field); \ } - if (disk->blkdeviotune.read_bytes_sec) { - virBufferAsprintf(&opt, ",bps_rd=%llu", - disk->blkdeviotune.read_bytes_sec); - } - - if (disk->blkdeviotune.write_bytes_sec) { - virBufferAsprintf(&opt, ",bps_wr=%llu", - disk->blkdeviotune.write_bytes_sec); - } - - if (disk->blkdeviotune.total_iops_sec) { - virBufferAsprintf(&opt, ",iops=%llu", - disk->blkdeviotune.total_iops_sec); - } - - if (disk->blkdeviotune.read_iops_sec) { - virBufferAsprintf(&opt, ",iops_rd=%llu", - disk->blkdeviotune.read_iops_sec); - } - - if (disk->blkdeviotune.write_iops_sec) { - virBufferAsprintf(&opt, ",iops_wr=%llu", - disk->blkdeviotune.write_iops_sec); - } - - if (disk->blkdeviotune.total_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_max=%llu", - disk->blkdeviotune.total_bytes_sec_max); - } - - if (disk->blkdeviotune.read_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_rd_max=%llu", - disk->blkdeviotune.read_bytes_sec_max); - } - - if (disk->blkdeviotune.write_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_wr_max=%llu", - disk->blkdeviotune.write_bytes_sec_max); - } + IOTUNE_ADD(total_bytes_sec, "bps-total"); + IOTUNE_ADD(read_bytes_sec, "bps-read"); + IOTUNE_ADD(write_bytes_sec, "bps-write"); + IOTUNE_ADD(total_iops_sec, "iops-total"); + IOTUNE_ADD(read_iops_sec, "iops-read"); + IOTUNE_ADD(write_iops_sec, "iops-write"); - if (disk->blkdeviotune.total_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_max=%llu", - disk->blkdeviotune.total_iops_sec_max); - } + IOTUNE_ADD(total_bytes_sec_max, "bps-total-max"); + IOTUNE_ADD(read_bytes_sec_max, "bps-read-max"); + IOTUNE_ADD(write_bytes_sec_max, "bps-write-max"); + IOTUNE_ADD(total_iops_sec_max, "iops-total-max"); + IOTUNE_ADD(read_iops_sec_max, "iops-read-max"); + IOTUNE_ADD(write_iops_sec_max, "iops-write-max"); - if (disk->blkdeviotune.read_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_rd_max=%llu", - disk->blkdeviotune.read_iops_sec_max); - } + IOTUNE_ADD(size_iops_sec, "iops-size"); - if (disk->blkdeviotune.write_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_wr_max=%llu", - disk->blkdeviotune.write_iops_sec_max); - } - - if (disk->blkdeviotune.size_iops_sec) { - virBufferAsprintf(&opt, ",iops_size=%llu", - disk->blkdeviotune.size_iops_sec); - } +#undef IOTUNE_ADD if (virBufferCheckError(&opt) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args index 66e1c10..58c15c8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args @@ -18,10 +18,14 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ -cache=none,bps=5000,iops=6000,bps_max=10000,iops_max=11000 \ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\ +throttling.bps-total-max=10000,throttling.iops-total-max=11000 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\ -cache=none,bps_rd=5000,bps_wr=5500,iops_rd=3500,iops_wr=4000,bps_rd_max=6000,\ -bps_wr_max=6500,iops_rd_max=7000,iops_wr_max=7500,iops_size=2000 \ +cache=none,throttling.bps-read=5000,throttling.bps-write=5500,\ +throttling.iops-read=3500,throttling.iops-write=4000,\ +throttling.bps-read-max=6000,throttling.bps-write-max=6500,\ +throttling.iops-read-max=7000,throttling.iops-write-max=7500,\ +throttling.iops-size=2000 \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args index 1f9983f..11833e6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args @@ -18,9 +18,10 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ -cache=none,bps=5000,iops=6000 \ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\ -cache=none,bps_rd=5000,bps_wr=5000,iops=7000 \ +cache=none,throttling.bps-read=5000,throttling.bps-write=5000,\ +throttling.iops-total=7000 \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 2.7.4
We're about to add more options, let's avoid having multiple if-then-else which each try to set up the qemuMonitorJSONMakeCommand call with all the parameters it knows about. Instead, use the fact that when a NULL is found in the argument list that processing of the remaining arguments stops and just have call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 49 ++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..43a3fa7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4587,35 +4587,26 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, virJSONValuePtr result = NULL; /* The qemu capability check has already been made in - * qemuDomainSetBlockIoTune */ - if (supportMaxOptions) { - cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", - "s:device", device, - "U:bps", info->total_bytes_sec, - "U:bps_rd", info->read_bytes_sec, - "U:bps_wr", info->write_bytes_sec, - "U:iops", info->total_iops_sec, - "U:iops_rd", info->read_iops_sec, - "U:iops_wr", info->write_iops_sec, - "U:bps_max", info->total_bytes_sec_max, - "U:bps_rd_max", info->read_bytes_sec_max, - "U:bps_wr_max", info->write_bytes_sec_max, - "U:iops_max", info->total_iops_sec_max, - "U:iops_rd_max", info->read_iops_sec_max, - "U:iops_wr_max", info->write_iops_sec_max, - "U:iops_size", info->size_iops_sec, - NULL); - } else { - cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", - "s:device", device, - "U:bps", info->total_bytes_sec, - "U:bps_rd", info->read_bytes_sec, - "U:bps_wr", info->write_bytes_sec, - "U:iops", info->total_iops_sec, - "U:iops_rd", info->read_iops_sec, - "U:iops_wr", info->write_iops_sec, - NULL); - } + * qemuDomainSetBlockIoTune. NB, once a NULL is found in + * the sequence, qemuMonitorJSONMakeCommand will stop. So + * let's make use of that when !supportMaxOptions */ + cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", + "s:device", device, + "U:bps", info->total_bytes_sec, + "U:bps_rd", info->read_bytes_sec, + "U:bps_wr", info->write_bytes_sec, + "U:iops", info->total_iops_sec, + "U:iops_rd", info->read_iops_sec, + "U:iops_wr", info->write_iops_sec, + "U:bps_max", info->total_bytes_sec_max, + !supportMaxOptions ? NULL : + "U:bps_rd_max", info->read_bytes_sec_max, + "U:bps_wr_max", info->write_bytes_sec_max, + "U:iops_max", info->total_iops_sec_max, + "U:iops_rd_max", info->read_iops_sec_max, + "U:iops_wr_max", info->write_iops_sec_max, + "U:iops_size", info->size_iops_sec, + NULL); if (!cmd) return -1; -- 2.7.4
Add new options to allow proving a duration/length in seconds to allow the bps/iops (and friends) to occur: total_bytes_sec_max_length write_bytes_sec_max_length read_bytes_sec_max_length total_iops_sec_max_length write_iops_sec_max_length read_iops_sec_max_length Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 48 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 86126d0..5744e25 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2321,6 +2321,54 @@ int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, # define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX "write_iops_sec_max" /** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by total_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH "total_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by read_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH "read_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by write_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH "write_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by total_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH "total_iops_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by read_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH "read_iops_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by write_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH "write_iops_sec_max" + +/** * VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC: * Macro for the BlockIoTune tunable weight: it represents the size * I/O operations per second permitted through a block device, as a ullong. -- 2.7.4
Add the capability to detect if the qemu binary can support the feature to use bps-max-length and friends. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + 7 files changed, 8 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d859c4..298aa33 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -344,6 +344,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-hotpluggable-cpus", "virtio-net.rx_queue_size", /* 235 */ + "drive-iotune-max-length", ); @@ -2852,6 +2853,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "name", "debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS }, { "name", "guest", QEMU_CAPS_NAME_GUEST }, { "spice", "unix", QEMU_CAPS_SPICE_UNIX }, + { "drive", "throttling.bps-total-max-length", QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ba0ef48..d99d61d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -378,6 +378,7 @@ typedef enum { /* 235 */ QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */ + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH, /* -drive bps_max_length = and friends */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml index fd14665..d5089dc 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -159,6 +159,7 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='drive-iotune-max-length'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml index eb708f8..3417996 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -159,6 +159,7 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='drive-iotune-max-length'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml index 482b384..6a653ae 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -153,6 +153,7 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='drive-iotune-max-length'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 60f1fcf..cbea49e 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -196,6 +196,7 @@ <flag name='intel-iommu'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='drive-iotune-max-length'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index 98f3762..d9e9a78 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -197,6 +197,7 @@ <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> <flag name='query-hotpluggable-cpus'/> + <flag name='drive-iotune-max-length'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0)</package> -- 2.7.4
Add support for a duration/length for the bps/iops and friends. Modify the API in order to add the "blkdeviotune." specific definitions for the iotune throttling duration/length options total_bytes_sec_max_length write_bytes_sec_max_length read_bytes_sec_max_length total_iops_sec_max_length write_iops_sec_max_length read_iops_sec_max_length Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 54 ++++++++++++++++++++ src/conf/domain_conf.h | 6 +++ src/qemu/qemu_driver.c | 106 ++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_monitor.c | 7 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 25 ++++++++- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 17 ++++++- 8 files changed, 211 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5744e25..beeedc6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3845,6 +3845,60 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec" /** + * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.total_bytes_sec_max, + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH "blkdeviotune.total_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.read_bytes_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH "blkdeviotune.read_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.write_bytes_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH "blkdeviotune.write_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.total_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH "blkdeviotune.total_iops_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.read_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH "blkdeviotune.read_iops_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.write_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH "blkdeviotune.write_iops_sec_max_length" + +/** * virConnectDomainEventTunableCallback: * @conn: connection object * @dom: domain on which the event occurred diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce90c27..2869951 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -548,6 +548,12 @@ struct _virDomainBlockIoTuneInfo { unsigned long long read_iops_sec_max; unsigned long long write_iops_sec_max; unsigned long long size_iops_sec; + unsigned long long total_bytes_sec_max_length; + unsigned long long read_bytes_sec_max_length; + unsigned long long write_bytes_sec_max_length; + unsigned long long total_iops_sec_max_length; + unsigned long long read_iops_sec_max_length; + unsigned long long write_iops_sec_max_length; }; typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..3b04754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,7 +113,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_MEM_PARAM 3 #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH 12 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 19 #define QEMU_NB_NUMA_PARAM 2 @@ -17262,7 +17263,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, bool set_bytes_max = false; bool set_iops_max = false; bool set_size_iops = false; + bool set_bytes_max_length = false; + bool set_iops_max_length = false; bool supportMaxOptions = true; + bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; virTypedParameterPtr eventParams = NULL; @@ -17298,6 +17302,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, NULL) < 0) return -1; @@ -17449,6 +17465,60 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, param->value.ul) < 0) goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { + info.total_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { + info.read_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { + info.write_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { + info.total_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { + info.read_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { + info.write_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; } } @@ -17496,6 +17566,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); + supportMaxLengthOptions = + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH); + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block I/O throttling not supported with this " @@ -17511,6 +17584,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + if (!supportMaxLengthOptions && + (set_iops_max_length || set_bytes_max_length)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a block I/O throttling length parameter is not " + "supported with this QEMU binary")); + goto endjob; + } + if (!(disk = qemuDomainDiskByName(def, path))) goto endjob; @@ -17543,6 +17624,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!set_size_iops) info.size_iops_sec = oldinfo->size_iops_sec; + if (!set_bytes_max_length) { + info.total_bytes_sec_max_length = oldinfo->total_bytes_sec_max_length; + info.read_bytes_sec_max_length = oldinfo->read_bytes_sec_max_length; + info.write_bytes_sec_max_length = oldinfo->write_bytes_sec_max_length; + } + if (!set_iops_max_length) { + info.total_iops_sec_max_length = oldinfo->total_iops_sec_max_length; + info.read_iops_sec_max_length = oldinfo->read_iops_sec_max_length; + info.write_iops_sec_max_length = oldinfo->write_iops_sec_max_length; + } #define CHECK_MAX(val) \ do { \ @@ -17566,7 +17657,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, - &info, supportMaxOptions); + &info, supportMaxOptions, + supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) @@ -17667,6 +17759,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH; } if (*nparams == 0) { @@ -17730,6 +17825,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); + BLOCK_IOTUNE_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, total_bytes_sec_max_length); + BLOCK_IOTUNE_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length); + BLOCK_IOTUNE_ASSIGN(WRITE_BYTES_SEC_MAX_LENGTH, write_bytes_sec_max_length); + + BLOCK_IOTUNE_ASSIGN(TOTAL_IOPS_SEC_MAX_LENGTH, total_iops_sec_max_length); + BLOCK_IOTUNE_ASSIGN(READ_IOPS_SEC_MAX_LENGTH, read_iops_sec_max_length); + BLOCK_IOTUNE_ASSIGN(WRITE_IOPS_SEC_MAX_LENGTH, write_iops_sec_max_length); #undef BLOCK_IOTUNE_ASSIGN ret = 0; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 8083a36..f800efd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3423,14 +3423,17 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions) + bool supportMaxOptions, + bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); QEMU_CHECK_MONITOR(mon); if (mon->json) - return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, supportMaxOptions); + return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, + supportMaxOptions, + supportMaxLengthOptions); else return qemuMonitorTextSetBlockIoThrottle(mon, device, info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7d78e5b..700dff9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -873,7 +873,8 @@ int qemuMonitorOpenGraphics(qemuMonitorPtr mon, int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions); + bool supportMaxOptions, + bool supportMaxLengthOptions); int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, const char *device, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 43a3fa7..6b26cf9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4559,6 +4559,12 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, GET_THROTTLE_STATS_OPTIONAL("iops_rd_max", read_iops_sec_max); GET_THROTTLE_STATS_OPTIONAL("iops_wr_max", write_iops_sec_max); GET_THROTTLE_STATS_OPTIONAL("iops_size", size_iops_sec); + GET_THROTTLE_STATS_OPTIONAL("bps_max_length", total_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("bps_rd_max_length", read_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("bps_wr_max_length", write_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_max_length", total_iops_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_rd_max_length", read_iops_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_wr_max_length", write_iops_sec_max_length); break; } @@ -4580,7 +4586,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions) + bool supportMaxOptions, + bool supportMaxLengthOptions) { int ret = -1; virJSONValuePtr cmd = NULL; @@ -4589,7 +4596,8 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, /* The qemu capability check has already been made in * qemuDomainSetBlockIoTune. NB, once a NULL is found in * the sequence, qemuMonitorJSONMakeCommand will stop. So - * let's make use of that when !supportMaxOptions */ + * let's make use of that when !supportMaxOptions and + * similarly when !supportMaxLengthOptions */ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", "s:device", device, "U:bps", info->total_bytes_sec, @@ -4606,6 +4614,19 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, "U:iops_rd_max", info->read_iops_sec_max, "U:iops_wr_max", info->write_iops_sec_max, "U:iops_size", info->size_iops_sec, + !supportMaxLengthOptions ? NULL : + "U:bps_max_length", + info->total_bytes_sec_max_length, + "U:bps_rd_max_length", + info->read_bytes_sec_max_length, + "U:bps_wr_max_length", + info->write_bytes_sec_max_length, + "U:iops_max_length", + info->total_iops_sec_max_length, + "U:iops_rd_max_length", + info->read_iops_sec_max_length, + "U:iops_wr_max_length", + info->write_iops_sec_max_length, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 6a5eb3b..77b2e02 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -327,7 +327,8 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions); + bool supportMaxOptions, + bool supportMaxLengthOptions); int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, const char *device, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index cbc39c6..d4f6250 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -67,6 +67,12 @@ const char *queryBlockReply = " \"iops_rd_max\": 11," " \"iops_wr_max\": 12," " \"iops_size\": 13," +" \"bps_max_length\": 14," +" \"bps_rd_max_length\": 15," +" \"bps_wr_max_length\": 16," +" \"iops_max_length\": 17," +" \"iops_rd_max_length\": 18," +" \"iops_wr_max_length\": 19," " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," " \"encryption_key_missing\": false" " }," @@ -1885,7 +1891,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1; - expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || qemuMonitorTestAddItemParams(test, "block_set_io_throttle", @@ -1897,6 +1903,12 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) "bps_wr_max", "9", "iops_max", "10", "iops_rd_max", "11", "iops_wr_max", "12", "iops_size", "13", + "bps_max_length", "14", + "bps_rd_max_length", "15", + "bps_wr_max_length", "16", + "iops_max_length", "17", + "iops_rd_max_length", "18", + "iops_wr_max_length", "19", NULL, NULL) < 0) goto cleanup; @@ -1911,7 +1923,8 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) } if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), - "drive-virtio-disk1", &info, true) < 0) + "drive-virtio-disk1", &info, true, + true) < 0) goto cleanup; ret = 0; -- 2.7.4
On 23.09.2016 14:56, John Ferlan wrote:
Add support for a duration/length for the bps/iops and friends.
Modify the API in order to add the "blkdeviotune." specific definitions for the iotune throttling duration/length options
total_bytes_sec_max_length write_bytes_sec_max_length read_bytes_sec_max_length total_iops_sec_max_length write_iops_sec_max_length read_iops_sec_max_length
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 54 ++++++++++++++++++++ src/conf/domain_conf.h | 6 +++ src/qemu/qemu_driver.c | 106 ++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_monitor.c | 7 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 25 ++++++++- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 17 ++++++- 8 files changed, 211 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..3b04754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,7 +113,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_MEM_PARAM 3
#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH 12 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 19
#define QEMU_NB_NUMA_PARAM 2
@@ -17262,7 +17263,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, bool set_bytes_max = false; bool set_iops_max = false; bool set_size_iops = false; + bool set_bytes_max_length = false; + bool set_iops_max_length = false; bool supportMaxOptions = true; + bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; virTypedParameterPtr eventParams = NULL; @@ -17298,6 +17302,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, + VIR_TYPED_PARAM_ULLONG, NULL) < 0) return -1;
@@ -17449,6 +17465,60 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, param->value.ul) < 0) goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { + info.total_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { + info.read_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { + info.write_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { + info.total_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { + info.read_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { + info.write_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob;
I wonder if we could turn these into something more readable. But that's something for a different patch set.
} }
Michal
+ } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { + info.write_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob;
I wonder if we could turn these into something more readable. But that's something for a different patch set.
Well since I didn't push before freeze maybe I can insert/add a patch that will make for fewer lines and push once 2.4 opens along with adjustments to change from 2.3 to 2.4. Initially I was scared off by the if-else if logic, but after working through the conditions, I ended up with 3 macros that build upon the 'bytes' and 'iops' in order to create each of the if/else checks. Starting with "bytes" or "iops", for each a prefix of "total_", "read_", or "write_" will be added prior to calling another macro which will append the postfixes of "_sec", "_sec_max", and "_sec_max_length". It looks cleaner, but really makes one look closely to see what's happening. I'll post a reply here with the patch for consideration John
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field. Signed-off-by: John Ferlan <jferlan@redhat.com> --- NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch Since I missed 2.3, I figured why not try to make the change. src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob; +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); + +/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec", + * "_sec_max", and "_sec_max_length" to each of the variables in order to + * in order to check each of the ways one of these can be used. The boolean + * doesn't care if "total_", "read_", or "write_" is the prefix. */ +#define CHECK_IOTUNE(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) { \ + SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length, \ + CONST##_SEC_MAX_LENGTH); \ + } + +/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto + * the field/const names, but also passing the "base" field to be used as the + * basis of the boolean. */ +#define CHECK_SET_IOTUNE(FIELD, CONST) \ + CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); \ + CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST); \ + CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST); + for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + /* Use the macros to check the different fields that can be set for + * bytes and iops. In the end 18 different combinations are tried. */ + CHECK_SET_IOTUNE(bytes, BYTES); + CHECK_SET_IOTUNE(iops, IOPS); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE_FIELD +#undef CHECK_IOTUNE +#undef CHECK_SET_IOTUNE if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) { -- 2.7.4
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
+/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec", + * "_sec_max", and "_sec_max_length" to each of the variables in order to + * in order to check each of the ways one of these can be used. The boolean + * doesn't care if "total_", "read_", or "write_" is the prefix. */ +#define CHECK_IOTUNE(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) { \ + SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length, \ + CONST##_SEC_MAX_LENGTH); \ + } + +/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto + * the field/const names, but also passing the "base" field to be used as the + * basis of the boolean. */ +#define CHECK_SET_IOTUNE(FIELD, CONST) \ + CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); \ + CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST); \ + CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST); +
I'm rather skeptic about ^^this because IMHO it's just too much in terms of code duplication cleanup. I think that in this case (like in many other cases) we should try to find the sweet spot between line removal and the time programmer spends reading these macros. We could use exactly the same approach as we do with STORE_MEM_RECORD, QEMU_ADD_NET_PARAM, PARSE_MEMTUNE_PARAM. Now, I know that in the original hunk below was a massive if-else if block which compared to a bunch of ifs like these: SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); would prevent testing of any extra conditions once a single condition has been satisfied. But I believe that, provided that param->field does not change within any of the 'if' blocks (which it doesn't), the compiler would optimize the two example hunks below and produce an identical assembly for both. if (foo == 0) bar0(); else if (foo == 1) bar1(); else if (foo == 2) bar2(); ################## if (foo == 0) bar0(); if (foo == 1) bar1(); if (foo == 2) bar2(); Erik
for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i];
@@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
- if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + /* Use the macros to check the different fields that can be set for + * bytes and iops. In the end 18 different combinations are tried. */ + CHECK_SET_IOTUNE(bytes, BYTES); + CHECK_SET_IOTUNE(iops, IOPS); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE_FIELD +#undef CHECK_IOTUNE +#undef CHECK_SET_IOTUNE
if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) {
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing: if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); felt like only a slight improvement to visibility. I agree what I ended up with I agree is overkill with respect to needing to stop, think, and map out what the macros do. I also know I went from if - else if - else if - else if (etc) to groups of 3 if - else if - else if. I was going to mention it, but figured it'd be somewhat obvious. But no harm in seeing what others thought - it's easy enough to go back to ^^this^^ version... BTW: eventually the SET_IOTUNE_FIELD spread across two lines especially with the _MAX_LENGTH parameters. Thanks for the feedback - I'll post a v2 in a little while... John
+/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec", + * "_sec_max", and "_sec_max_length" to each of the variables in order to + * in order to check each of the ways one of these can be used. The boolean + * doesn't care if "total_", "read_", or "write_" is the prefix. */ +#define CHECK_IOTUNE(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) { \ + SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length, \ + CONST##_SEC_MAX_LENGTH); \ + } + +/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto + * the field/const names, but also passing the "base" field to be used as the + * basis of the boolean. */ +#define CHECK_SET_IOTUNE(FIELD, CONST) \ + CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); \ + CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST); \ + CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST); +
I'm rather skeptic about ^^this because IMHO it's just too much in terms of code duplication cleanup. I think that in this case (like in many other cases) we should try to find the sweet spot between line removal and the time programmer spends reading these macros. We could use exactly the same approach as we do with STORE_MEM_RECORD, QEMU_ADD_NET_PARAM, PARSE_MEMTUNE_PARAM. Now, I know that in the original hunk below was a massive if-else if block which compared to a bunch of ifs like these:
SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3);
would prevent testing of any extra conditions once a single condition has been satisfied. But I believe that, provided that param->field does not change within any of the 'if' blocks (which it doesn't), the compiler would optimize the two example hunks below and produce an identical assembly for both.
if (foo == 0) bar0(); else if (foo == 1) bar1(); else if (foo == 2) bar2(); ################## if (foo == 0) bar0(); if (foo == 1) bar1(); if (foo == 2) bar2();
Erik
for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i];
@@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
- if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + /* Use the macros to check the different fields that can be set for + * bytes and iops. In the end 18 different combinations are tried. */ + CHECK_SET_IOTUNE(bytes, BYTES); + CHECK_SET_IOTUNE(iops, IOPS); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE_FIELD +#undef CHECK_IOTUNE +#undef CHECK_SET_IOTUNE
if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) {
On 30/09/16 12:57, John Ferlan wrote:
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable: SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think. Erik
felt like only a slight improvement to visibility. I agree what I ended up with I agree is overkill with respect to needing to stop, think, and map out what the macros do. I also know I went from if - else if - else if - else if (etc) to groups of 3 if - else if - else if. I was going to mention it, but figured it'd be somewhat obvious.
But no harm in seeing what others thought - it's easy enough to go back to ^^this^^ version... BTW: eventually the SET_IOTUNE_FIELD spread across two lines especially with the _MAX_LENGTH parameters.
Thanks for the feedback - I'll post a v2 in a little while...
John
+/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec", + * "_sec_max", and "_sec_max_length" to each of the variables in order to + * in order to check each of the ways one of these can be used. The boolean + * doesn't care if "total_", "read_", or "write_" is the prefix. */ +#define CHECK_IOTUNE(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) { \ + SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) { \ + SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length, \ + CONST##_SEC_MAX_LENGTH); \ + } + +/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto + * the field/const names, but also passing the "base" field to be used as the + * basis of the boolean. */ +#define CHECK_SET_IOTUNE(FIELD, CONST) \ + CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); \ + CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST); \ + CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST); +
I'm rather skeptic about ^^this because IMHO it's just too much in terms of code duplication cleanup. I think that in this case (like in many other cases) we should try to find the sweet spot between line removal and the time programmer spends reading these macros. We could use exactly the same approach as we do with STORE_MEM_RECORD, QEMU_ADD_NET_PARAM, PARSE_MEMTUNE_PARAM. Now, I know that in the original hunk below was a massive if-else if block which compared to a bunch of ifs like these:
SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3);
would prevent testing of any extra conditions once a single condition has been satisfied. But I believe that, provided that param->field does not change within any of the 'if' blocks (which it doesn't), the compiler would optimize the two example hunks below and produce an identical assembly for both.
if (foo == 0) bar0(); else if (foo == 1) bar1(); else if (foo == 2) bar2(); ################## if (foo == 0) bar0(); if (foo == 1) bar1(); if (foo == 2) bar2();
Erik
for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i];
@@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
- if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + /* Use the macros to check the different fields that can be set for + * bytes and iops. In the end 18 different combinations are tried. */ + CHECK_SET_IOTUNE(bytes, BYTES); + CHECK_SET_IOTUNE(iops, IOPS); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE_FIELD +#undef CHECK_IOTUNE +#undef CHECK_SET_IOTUNE
if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) {
On 09/30/2016 07:08 AM, Erik Skultety wrote:
On 30/09/16 12:57, John Ferlan wrote:
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable:
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think.
So essentially I end up with two macros and 7 calls to those macros: #define SET_IOTUNE(FIELD, BOOL, CONST) \ do { \ info.FIELD = params->value.ul; \ set_##BOOL = true; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ param->value.ul) < 0) \ goto endjob; \ } while (0); #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ SET_IOTUNE(FIELD, BOOL, CONST); \ } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ } else if (STREQ(param->field, \ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ } SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC); #undef SET_IOTUNE #undef SET_IOTUNE_FIELD
On 30/09/16 13:47, John Ferlan wrote:
On 09/30/2016 07:08 AM, Erik Skultety wrote:
On 30/09/16 12:57, John Ferlan wrote:
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable:
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think.
So essentially I end up with two macros and 7 calls to those macros:
#define SET_IOTUNE(FIELD, BOOL, CONST) \ do { \ info.FIELD = params->value.ul; \ set_##BOOL = true; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ param->value.ul) < 0) \ goto endjob; \ } while (0);
#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ SET_IOTUNE(FIELD, BOOL, CONST); \ } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ } else if (STREQ(param->field, \ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ }
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC);
#undef SET_IOTUNE #undef SET_IOTUNE_FIELD
Almost, but what I had in mind was to just keep one of the macros - be it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then end up with a snippet like the one in the attached patch. Erik PS: Sorry for attaching a patch, but thunderbird's editor truly sucks (pasting from vim is a total nightmare)...big time...and I really have to switch to some normal mail client.
On 10/03/2016 03:56 AM, Erik Skultety wrote:
On 30/09/16 13:47, John Ferlan wrote:
On 09/30/2016 07:08 AM, Erik Skultety wrote:
On 30/09/16 12:57, John Ferlan wrote:
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote:
Rework the code in a set of 3 macros that will use the "base" of 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', and 'write_' before adding the postfixes of '_sec', '_sec_max', and '_sec_max_length' and making the param->field comparison and adding of the field.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
NB: Hopefully this applies - my branch is based off of the git head which I refreshed prior to sending the patch
Since I missed 2.3, I figured why not try to make the change.
src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- 1 file changed, 45 insertions(+), 171 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b5b6fc..cbf9483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable:
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think.
So essentially I end up with two macros and 7 calls to those macros:
#define SET_IOTUNE(FIELD, BOOL, CONST) \ do { \ info.FIELD = params->value.ul; \ set_##BOOL = true; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ param->value.ul) < 0) \ goto endjob; \ } while (0);
#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ SET_IOTUNE(FIELD, BOOL, CONST); \ } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ } else if (STREQ(param->field, \ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ }
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC);
#undef SET_IOTUNE #undef SET_IOTUNE_FIELD
Almost, but what I had in mind was to just keep one of the macros - be it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then end up with a snippet like the one in the attached patch.
Erik
PS: Sorry for attaching a patch, but thunderbird's editor truly sucks (pasting from vim is a total nightmare)...big time...and I really have to switch to some normal mail client.
If you 'temporarily' turn off line wrapping for t-bird, then cut-n-paste isn't too bad, but attaching a patch was fine. Suffice to say what you propose is where I initially started. Then I looked at the extended repetition of lines where the only difference was "_max"/"_MAX" and "_max_length"/"_MAX_LENGTH" and felt it was better to further collapse the way I did... In any case, see patch 10 for another instance of "parsing" 2 things at a time (PARSE_IOTUNE) and patch 11 for adding the _max_length to that. In fact the patch 11 shows my entrails with the commented out macros (since removed from my branch). I chose not do the same in patch 4 (IOTUNE_ADD) or patch 9 (FORMAT_IOTUNE) because I didn't want to mess with the .args or .xml output "adjustments" (e.g. output files). Personally I'd rather see the 3 variables close to each other rather than hunting through the various output trying to see which are set. Way too similarity that can "fool" the eyes. I'd prefer to keep the version of the macro I have since it doesn't affect the output/visible order. John
On 03/10/16 14:16, John Ferlan wrote:
On 10/03/2016 03:56 AM, Erik Skultety wrote:
On 30/09/16 13:47, John Ferlan wrote:
On 09/30/2016 07:08 AM, Erik Skultety wrote:
On 30/09/16 12:57, John Ferlan wrote:
On 09/30/2016 04:43 AM, Erik Skultety wrote:
On 28/09/16 22:27, John Ferlan wrote: > Rework the code in a set of 3 macros that will use the "base" of > 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_', > and 'write_' before adding the postfixes of '_sec', '_sec_max', > and '_sec_max_length' and making the param->field comparison and > adding of the field. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > > NB: Hopefully this applies - my branch is based off of the git head > which I refreshed prior to sending the patch > > Since I missed 2.3, I figured why not try to make the change. > > src/qemu/qemu_driver.c | 216 +++++++++++-------------------------------------- > 1 file changed, 45 insertions(+), 171 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 2b5b6fc..cbf9483 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) > goto endjob; > > +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ > + do { \ > + info.FIELD = params->value.ul; \ > + BOOL = true; \ > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ > + &eventMaxparams, \ > + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ > + param->value.ul) < 0) \ > + goto endjob; \ > + } while (0); > +
While I totally support ^^this kind of macro-based code cleanup,
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable:
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think.
So essentially I end up with two macros and 7 calls to those macros:
#define SET_IOTUNE(FIELD, BOOL, CONST) \ do { \ info.FIELD = params->value.ul; \ set_##BOOL = true; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ param->value.ul) < 0) \ goto endjob; \ } while (0);
#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ SET_IOTUNE(FIELD, BOOL, CONST); \ } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ } else if (STREQ(param->field, \ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ }
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC);
#undef SET_IOTUNE #undef SET_IOTUNE_FIELD
Almost, but what I had in mind was to just keep one of the macros - be it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then end up with a snippet like the one in the attached patch.
Erik
PS: Sorry for attaching a patch, but thunderbird's editor truly sucks (pasting from vim is a total nightmare)...big time...and I really have to switch to some normal mail client.
If you 'temporarily' turn off line wrapping for t-bird, then cut-n-paste isn't too bad, but attaching a patch was fine.
Yeah, I could have done that, adjusting the message I sent along with the patch (I also tried selective wrap but that certainly didn't bring satisfactory results). Instead, I rage quit and thought that attaching the patch might not just be the worst possible way at all. Anyway, I'm starting to convince myself that spending several hours configuring mutt might as well turn out as a plausible solution of all email-related future problems than sticking with thunderbird after all.
Suffice to say what you propose is where I initially started. Then I looked at the extended repetition of lines where the only difference was "_max"/"_MAX" and "_max_length"/"_MAX_LENGTH" and felt it was better to further collapse the way I did...
In any case, see patch 10 for another instance of "parsing" 2 things at a time (PARSE_IOTUNE) and patch 11 for adding the _max_length to that. In fact the patch 11 shows my entrails with the commented out macros (since removed from my branch).
I chose not do the same in patch 4 (IOTUNE_ADD) or patch 9 (FORMAT_IOTUNE) because I didn't want to mess with the .args or .xml output "adjustments" (e.g. output files).
Personally I'd rather see the 3 variables close to each other rather than hunting through the various output trying to see which are set. Way too similarity that can "fool" the eyes.
I'd prefer to keep the version of the macro I have since it doesn't affect the output/visible order.
John
Well, I do see your point, I just have a different feeling about that. When I look at the qemuDomainSetBlockIoTune function, I see a bunch of variables defined, among which all the set_(bytes|iops) variables reside. Let's consider a massive function where you introduced a macro to get rid of all the duplicate code. What I expect from the macro is that I clearly see what parameters it takes so I can very simply identify which variables will be affected by the macro whatever it is trying to accomplish. By introducing another macro representing an additional meta layer of name processing, I fail to see what arguments will be eventually affected by the macro machinery at first glance. Your initial idea which was identical to my proposal makes it clear what variables will be affected by the macro and the only thing that needs to be handled is prefixing the enums with VIR_DOMAIN_BLOCK_IOTUNE. In my opinion, by visually grouping the related blocks of code can also lead to certain amount of cleanliness. Another thing, since in patch 4 and 9 you went the way I'm currently describing, I'm not a fan of treating essentially the same thing differently. Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
Almost, but what I had in mind was to just keep one of the macros - be it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then end up with a snippet like the one in the attached patch.
Erik
PS: Sorry for attaching a patch, but thunderbird's editor truly sucks (pasting from vim is a total nightmare)...big time...and I really have to switch to some normal mail client.
If you 'temporarily' turn off line wrapping for t-bird, then cut-n-paste isn't too bad, but attaching a patch was fine.
Yeah, I could have done that, adjusting the message I sent along with the patch (I also tried selective wrap but that certainly didn't bring satisfactory results). Instead, I rage quit and thought that attaching the patch might not just be the worst possible way at all. Anyway, I'm starting to convince myself that spending several hours configuring mutt might as well turn out as a plausible solution of all email-related future problems than sticking with thunderbird after all.
You're young, make the change... I think you have plenty of "tech support" around you in Brno to get through all the 'gotchas'! Learning a new email client could take me much longer ;-)
Suffice to say what you propose is where I initially started. Then I looked at the extended repetition of lines where the only difference was "_max"/"_MAX" and "_max_length"/"_MAX_LENGTH" and felt it was better to further collapse the way I did...
In any case, see patch 10 for another instance of "parsing" 2 things at a time (PARSE_IOTUNE) and patch 11 for adding the _max_length to that. In fact the patch 11 shows my entrails with the commented out macros (since removed from my branch).
I chose not do the same in patch 4 (IOTUNE_ADD) or patch 9 (FORMAT_IOTUNE) because I didn't want to mess with the .args or .xml output "adjustments" (e.g. output files).
Personally I'd rather see the 3 variables close to each other rather than hunting through the various output trying to see which are set. Way too similarity that can "fool" the eyes.
I'd prefer to keep the version of the macro I have since it doesn't affect the output/visible order.
John
Well, I do see your point, I just have a different feeling about that. When I look at the qemuDomainSetBlockIoTune function, I see a bunch of variables defined, among which all the set_(bytes|iops) variables reside. Let's consider a massive function where you introduced a macro to get rid of all the duplicate code. What I expect from the macro is that I clearly see what parameters it takes so I can very simply identify which variables will be affected by the macro whatever it is trying to accomplish. By introducing another macro representing an additional meta layer of name processing, I fail to see what arguments will be eventually affected by the macro machinery at first glance. Your initial idea which was identical to my proposal makes it clear what variables will be affected by the macro and the only thing that needs to be handled is prefixing the enums with VIR_DOMAIN_BLOCK_IOTUNE. In my opinion, by visually grouping the related blocks of code can also lead to certain amount of cleanliness. Another thing, since in patch 4 and 9 you went the way I'm currently describing, I'm not a fan of treating essentially the same thing differently.
I flipped a coin and it came up heads, which means I'll adjust this code to be more like what you propose. I'll also likewise adjust patch 10 and 11 to PARSE_IOTUNE each variable - just to keep things consistent. Now if I can only work through the RPC issue I put in a reply to .0 <sigh> Tks - John
Rework the code to create a couple of macros to hide all the comparisons for each of the 6 "types" of data fields to collect. Each macro will check the base name, then the base name + "_max", and base name + "_max_length". Signed-off-by: John Ferlan <jferlan@redhat.com> --- "Formalized" patch based on the 08.5/12 conversation src/qemu/qemu_driver.c | 203 ++++++++----------------------------------------- 1 file changed, 32 insertions(+), 171 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5322405..9e9c8d6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17377,6 +17377,27 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob; +#define SET_IOTUNE(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + set_##BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); + +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ + SET_IOTUNE(FIELD, BOOL, CONST); \ + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ + SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ + SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ + } + for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -17387,178 +17408,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); + SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); + SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); + SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); + SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); + SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE +#undef SET_IOTUNE_FIELD if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) { -- 2.7.4
On 09/23/2016 08:56 AM, John Ferlan wrote:
Add support for a duration/length for the bps/iops and friends.
Modify the API in order to add the "blkdeviotune." specific definitions for the iotune throttling duration/length options
total_bytes_sec_max_length write_bytes_sec_max_length read_bytes_sec_max_length total_iops_sec_max_length write_iops_sec_max_length read_iops_sec_max_length
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 54 ++++++++++++++++++++ src/conf/domain_conf.h | 6 +++ src/qemu/qemu_driver.c | 106 ++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_monitor.c | 7 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 25 ++++++++- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 17 ++++++- 8 files changed, 211 insertions(+), 10 deletions(-)
While working on the "next" iotune tunable to add I realized something... [...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..3b04754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,7 +113,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_MEM_PARAM 3
#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH 12 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 19
These need to be: #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19 Then [...]
@@ -17667,6 +17759,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH; }
This should be : if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; The default is 19 (get everything)... If we don't have the IOTUNE_MAX cap, then there's only 6 parameters to get. If we don't have the IOTUNE_MAX_LENGTH cap, then there's only 13 parameters to get. John
Rather than copy-paste - use a macro Unfortunately due to how the RNG schema was written keeping the 'value' and 'value'_max next to each other in the XML causes a schema failure, so the FORMAT has to write out singly rather than optimizing to write out both values at once Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 77 +++++++++++++------------------------------------- 1 file changed, 19 insertions(+), 58 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd34cec..f53d6c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20061,6 +20061,12 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, } +#define FORMAT_IOTUNE(val) \ + if (def->blkdeviotune.val) { \ + virBufferAsprintf(buf, "<" #val ">%llu</" #val ">\n", \ + def->blkdeviotune.val); \ + } + static int virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, @@ -20248,66 +20254,20 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec) { virBufferAddLit(buf, "<iotune>\n"); virBufferAdjustIndent(buf, 2); - if (def->blkdeviotune.total_bytes_sec) { - virBufferAsprintf(buf, "<total_bytes_sec>%llu</total_bytes_sec>\n", - def->blkdeviotune.total_bytes_sec); - } - - if (def->blkdeviotune.read_bytes_sec) { - virBufferAsprintf(buf, "<read_bytes_sec>%llu</read_bytes_sec>\n", - def->blkdeviotune.read_bytes_sec); - - } - - if (def->blkdeviotune.write_bytes_sec) { - virBufferAsprintf(buf, "<write_bytes_sec>%llu</write_bytes_sec>\n", - def->blkdeviotune.write_bytes_sec); - } - - if (def->blkdeviotune.total_iops_sec) { - virBufferAsprintf(buf, "<total_iops_sec>%llu</total_iops_sec>\n", - def->blkdeviotune.total_iops_sec); - } - - if (def->blkdeviotune.read_iops_sec) { - virBufferAsprintf(buf, "<read_iops_sec>%llu</read_iops_sec>\n", - def->blkdeviotune.read_iops_sec); - } - if (def->blkdeviotune.write_iops_sec) { - virBufferAsprintf(buf, "<write_iops_sec>%llu</write_iops_sec>\n", - def->blkdeviotune.write_iops_sec); - } - - if (def->blkdeviotune.total_bytes_sec_max) { - virBufferAsprintf(buf, "<total_bytes_sec_max>%llu</total_bytes_sec_max>\n", - def->blkdeviotune.total_bytes_sec_max); - } - - if (def->blkdeviotune.read_bytes_sec_max) { - virBufferAsprintf(buf, "<read_bytes_sec_max>%llu</read_bytes_sec_max>\n", - def->blkdeviotune.read_bytes_sec_max); - } - - if (def->blkdeviotune.write_bytes_sec_max) { - virBufferAsprintf(buf, "<write_bytes_sec_max>%llu</write_bytes_sec_max>\n", - def->blkdeviotune.write_bytes_sec_max); - } - - if (def->blkdeviotune.total_iops_sec_max) { - virBufferAsprintf(buf, "<total_iops_sec_max>%llu</total_iops_sec_max>\n", - def->blkdeviotune.total_iops_sec_max); - } + FORMAT_IOTUNE(total_bytes_sec); + FORMAT_IOTUNE(read_bytes_sec); + FORMAT_IOTUNE(write_bytes_sec); + FORMAT_IOTUNE(total_iops_sec); + FORMAT_IOTUNE(read_iops_sec); + FORMAT_IOTUNE(write_iops_sec); - if (def->blkdeviotune.read_iops_sec_max) { - virBufferAsprintf(buf, "<read_iops_sec_max>%llu</read_iops_sec_max>\n", - def->blkdeviotune.read_iops_sec_max); - } - - if (def->blkdeviotune.write_iops_sec_max) { - virBufferAsprintf(buf, "<write_iops_sec_max>%llu</write_iops_sec_max>\n", - def->blkdeviotune.write_iops_sec_max); - } + FORMAT_IOTUNE(total_bytes_sec_max); + FORMAT_IOTUNE(read_bytes_sec_max); + FORMAT_IOTUNE(write_bytes_sec_max); + FORMAT_IOTUNE(total_iops_sec_max); + FORMAT_IOTUNE(read_iops_sec_max); + FORMAT_IOTUNE(write_iops_sec_max); if (def->blkdeviotune.size_iops_sec) { virBufferAsprintf(buf, "<size_iops_sec>%llu</size_iops_sec>\n", @@ -20339,6 +20299,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</disk>\n"); return 0; } +#undef FORMAT_IOTUNE static int -- 2.7.4
Unlike formatting we can parse in any order - this just makes it look like there are less lines. A subsequent patch will add 6 more fields to parse and that will look better. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f53d6c5..861a15d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7127,6 +7127,13 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_XML_ERROR, \ _("disk iotune field '%s' must be an integer"), #val); \ return -1; \ + } \ + if (virXPathULongLong("string(./iotune/" #val "_max)", \ + ctxt, &def->blkdeviotune.val##_max) == -2) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune field '%s_max' must be an integer"), \ + #val); \ + return -1; \ } static int @@ -7140,14 +7147,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, PARSE_IOTUNE(read_iops_sec); PARSE_IOTUNE(write_iops_sec); - PARSE_IOTUNE(total_bytes_sec_max); - PARSE_IOTUNE(read_bytes_sec_max); - PARSE_IOTUNE(write_bytes_sec_max); - PARSE_IOTUNE(total_iops_sec_max); - PARSE_IOTUNE(read_iops_sec_max); - PARSE_IOTUNE(write_iops_sec_max); - - PARSE_IOTUNE(size_iops_sec); + if (virXPathULongLong("string(./iotune/size_iops_sec)", + ctxt, &def->blkdeviotune.size_iops_sec) == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk iotune field 'size_iops_sec' must be " + "an integer")); + return -1; + } if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || -- 2.7.4
Modify _virDomainBlockIoTuneInfo and rng schema to support the _length options for bps/iops throttling values. Document the new values. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 40 ++++++++++++- docs/schemas/domaincommon.rng | 38 +++++++++++++ src/conf/domain_conf.c | 45 ++++++++++++++- .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f48a4d8..d88f084 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2616,7 +2616,45 @@ maximum write I/O operations per second.</dd> <dt><code>size_iops_sec</code></dt> <dd>The optional <code>size_iops_sec</code> element is the - size of I/O operations per second.</dd> + size of I/O operations per second. + <p> + <span class="since">Throughput limits since 1.2.11 and QEMU 1.7</span> + </p> + </dd> + <dt><code>total_bytes_sec_max_length</code></dt> + <dd>The optional <code>total_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>total_bytes_sec_max</code> burst period. Only valid + when the <code>total_bytes_sec_max</code> is set.</dd> + <dt><code>read_bytes_sec_max_length</code></dt> + <dd>The optional <code>read_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>read_bytes_sec_max</code> burst period. Only valid + when the <code>read_bytes_sec_max</code> is set.</dd> + <dt><code>write_bytes_sec_max</code></dt> + <dd>The optional <code>write_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>write_bytes_sec_max</code> burst period. Only valid + when the if <code>write_bytes_sec_max</code> is set.</dd> + <dt><code>total_iops_sec_max_length</code></dt> + <dd>The optional <code>total_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>total_iops_sec_max</code> burst period. Only valid + when the <code>total_iops_sec_max</code> is set.</dd> + <dt><code>read_iops_sec_max_length</code></dt> + <dd>The optional <code>read_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>read_iops_sec_max</code> burst period. Only valid + when the <code>read_iops_sec_max</code> is set.</dd> + <dt><code>write_iops_sec_max</code></dt> + <dd>The optional <code>write_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>write_iops_sec_max</code> burst period. Only valid + when the <code>write_iops_sec_max</code> is set. + <p> + <span class="since">Throughput length since 2.3.0 and QEMU 2.6</span> + </p> + </dd> </dl> </dd> <dt><code>driver</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 95c7882..0c3a411 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4949,6 +4949,44 @@ <data type="unsignedLong"/> </element> </optional> + <choice> + <element name="total_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> + <choice> + <element name="total_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 861a15d..6e405c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7134,6 +7134,21 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, _("disk iotune field '%s_max' must be an integer"), \ #val); \ return -1; \ + } \ + if (virXPathULongLong("string(./iotune/" #val "_max_length)", \ + ctxt, &def->blkdeviotune.val##_max_length) == -2) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune field '%s_max_length' must be " \ + "an integer"), #val); \ + return -1; \ + } + +#define CHECK_IOTUNE_MAX_LENGTH(val) \ + if (def->blkdeviotune.val##_length && !def->blkdeviotune.val) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune '%s'_length cannot be set unless " \ + "disk iotune '%s' is also set"), #val, #val); \ + return -1; \ } static int @@ -7155,6 +7170,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; } + //PARSE_IOTUNE(total_bytes_sec_max_length); + //PARSE_IOTUNE(read_bytes_sec_max_length); + //PARSE_IOTUNE(write_bytes_sec_max_length); + //PARSE_IOTUNE(total_iops_sec_max_length); + //PARSE_IOTUNE(read_iops_sec_max_length); + //PARSE_IOTUNE(write_iops_sec_max_length); + if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || (def->blkdeviotune.total_bytes_sec && @@ -7195,8 +7217,16 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; } + CHECK_IOTUNE_MAX_LENGTH(total_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(read_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(write_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(total_iops_sec_max); + CHECK_IOTUNE_MAX_LENGTH(read_iops_sec_max); + CHECK_IOTUNE_MAX_LENGTH(write_iops_sec_max); + return 0; } +#undef CHECK_IOTUNE_MAX_LENGTH #undef PARSE_IOTUNE @@ -20257,7 +20287,13 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.total_iops_sec_max || def->blkdeviotune.read_iops_sec_max || def->blkdeviotune.write_iops_sec_max || - def->blkdeviotune.size_iops_sec) { + def->blkdeviotune.size_iops_sec || + def->blkdeviotune.total_bytes_sec_max_length || + def->blkdeviotune.read_bytes_sec_max_length || + def->blkdeviotune.write_bytes_sec_max_length || + def->blkdeviotune.total_iops_sec_max_length || + def->blkdeviotune.read_iops_sec_max_length || + def->blkdeviotune.write_iops_sec_max_length) { virBufferAddLit(buf, "<iotune>\n"); virBufferAdjustIndent(buf, 2); @@ -20280,6 +20316,13 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); } + FORMAT_IOTUNE(total_bytes_sec_max_length); + FORMAT_IOTUNE(read_bytes_sec_max_length); + FORMAT_IOTUNE(write_bytes_sec_max_length); + FORMAT_IOTUNE(total_iops_sec_max_length); + FORMAT_IOTUNE(read_iops_sec_max_length); + FORMAT_IOTUNE(write_iops_sec_max_length); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</iotune>\n"); } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml new file mode 100644 index 0000000..c740751 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml @@ -0,0 +1,65 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>5000</total_bytes_sec> + <total_iops_sec>6000</total_iops_sec> + <total_bytes_sec_max>10000</total_bytes_sec_max> + <total_iops_sec_max>11000</total_iops_sec_max> + <total_bytes_sec_max_length>3</total_bytes_sec_max_length> + <total_iops_sec_max_length>5</total_iops_sec_max_length> + </iotune> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <iotune> + <read_bytes_sec>5000</read_bytes_sec> + <write_bytes_sec>5500</write_bytes_sec> + <read_iops_sec>3500</read_iops_sec> + <write_iops_sec>4000</write_iops_sec> + <read_bytes_sec_max>6000</read_bytes_sec_max> + <write_bytes_sec_max>6500</write_bytes_sec_max> + <read_iops_sec_max>7000</read_iops_sec_max> + <write_iops_sec_max>7500</write_iops_sec_max> + <size_iops_sec>2000</size_iops_sec> + <read_bytes_sec_max_length>3</read_bytes_sec_max_length> + <write_bytes_sec_max_length>5</write_bytes_sec_max_length> + <read_iops_sec_max_length>7</read_iops_sec_max_length> + <write_iops_sec_max_length>9</write_iops_sec_max_length> + </iotune> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml new file mode 120000 index 0000000..67f3e9a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4b58986..5c50512 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -608,6 +608,7 @@ mymain(void) DO_TEST("usb-redir-filter-version", NONE); DO_TEST("blkdeviotune", NONE); DO_TEST("blkdeviotune-max", NONE); + DO_TEST("blkdeviotune-max-length", NONE); DO_TEST("controller-usb-order", NONE); DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); -- 2.7.4
On 23.09.2016 14:56, John Ferlan wrote:
Modify _virDomainBlockIoTuneInfo and rng schema to support the _length options for bps/iops throttling values. Document the new values.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 40 ++++++++++++- docs/schemas/domaincommon.rng | 38 +++++++++++++ src/conf/domain_conf.c | 45 ++++++++++++++- .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 861a15d..6e405c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7134,6 +7134,21 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, _("disk iotune field '%s_max' must be an integer"), \ #val); \ return -1; \ + } \ + if (virXPathULongLong("string(./iotune/" #val "_max_length)", \ + ctxt, &def->blkdeviotune.val##_max_length) == -2) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune field '%s_max_length' must be " \ + "an integer"), #val); \ + return -1; \ + } + +#define CHECK_IOTUNE_MAX_LENGTH(val) \ + if (def->blkdeviotune.val##_length && !def->blkdeviotune.val) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune '%s'_length cannot be set unless " \ + "disk iotune '%s' is also set"), #val, #val); \ + return -1; \ }
static int @@ -7155,6 +7170,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; }
+ //PARSE_IOTUNE(total_bytes_sec_max_length); + //PARSE_IOTUNE(read_bytes_sec_max_length); + //PARSE_IOTUNE(write_bytes_sec_max_length); + //PARSE_IOTUNE(total_iops_sec_max_length); + //PARSE_IOTUNE(read_iops_sec_max_length); + //PARSE_IOTUNE(write_iops_sec_max_length); +
Whoa, probably a leftover from development? Drop it.
if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || (def->blkdeviotune.total_bytes_sec &&
Michal
On 23.09.2016 14:56, John Ferlan wrote:
Modify _virDomainBlockIoTuneInfo and rng schema to support the _length options for bps/iops throttling values. Document the new values.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 40 ++++++++++++- docs/schemas/domaincommon.rng | 38 +++++++++++++ src/conf/domain_conf.c | 45 ++++++++++++++- .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 861a15d..6e405c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7134,6 +7134,21 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, _("disk iotune field '%s_max' must be an integer"), \ #val); \ return -1; \ + } \ + if (virXPathULongLong("string(./iotune/" #val "_max_length)", \ + ctxt, &def->blkdeviotune.val##_max_length) == -2) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune field '%s_max_length' must be " \ + "an integer"), #val); \ + return -1; \ + } + +#define CHECK_IOTUNE_MAX_LENGTH(val) \ + if (def->blkdeviotune.val##_length && !def->blkdeviotune.val) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune '%s'_length cannot be set unless " \ + "disk iotune '%s' is also set"), #val, #val); \ + return -1; \ }
static int @@ -7155,6 +7170,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; }
+ //PARSE_IOTUNE(total_bytes_sec_max_length); + //PARSE_IOTUNE(read_bytes_sec_max_length); + //PARSE_IOTUNE(write_bytes_sec_max_length); + //PARSE_IOTUNE(total_iops_sec_max_length); + //PARSE_IOTUNE(read_iops_sec_max_length); + //PARSE_IOTUNE(write_iops_sec_max_length); +
Whoa, probably a leftover from development? Drop it.
if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || (def->blkdeviotune.total_bytes_sec &&
Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1349898 Add in the block I/O throttling length/duration parameter to the command line if supported. If not supported, fail command creation. Add the xml2argvtest for testing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 21 +++++++++++++ .../qemuxml2argv-blkdeviotune-max-length.args | 34 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20fe6fc..4fa3d34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1747,6 +1747,20 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, goto error; } + /* block I/O throttling length 2.6 */ + if ((disk->blkdeviotune.total_bytes_sec_max_length || + disk->blkdeviotune.read_bytes_sec_max_length || + disk->blkdeviotune.write_bytes_sec_max_length || + disk->blkdeviotune.total_iops_sec_max_length || + disk->blkdeviotune.read_iops_sec_max_length || + disk->blkdeviotune.write_iops_sec_max_length) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("there are some block I/O throttling length parameters " + "that are not supported with this QEMU binary")); + goto error; + } + if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || @@ -1788,6 +1802,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, IOTUNE_ADD(size_iops_sec, "iops-size"); + IOTUNE_ADD(total_bytes_sec_max_length, "bps-total-max-length"); + IOTUNE_ADD(read_bytes_sec_max_length, "bps-read-max-length"); + IOTUNE_ADD(write_bytes_sec_max_length, "bps-write-max-length"); + IOTUNE_ADD(total_iops_sec_max_length, "iops-total-max-length"); + IOTUNE_ADD(read_iops_sec_max_length, "iops-read-max-length"); + IOTUNE_ADD(write_iops_sec_max_length, "iops-write-max-length"); + #undef IOTUNE_ADD if (virBufferCheckError(&opt) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args new file mode 100644 index 0000000..b656aea --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args @@ -0,0 +1,34 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\ +throttling.bps-total-max=10000,throttling.iops-total-max=11000,\ +throttling.bps-total-max-length=3,throttling.iops-total-max-length=5 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\ +cache=none,throttling.bps-read=5000,throttling.bps-write=5500,\ +throttling.iops-read=3500,throttling.iops-write=4000,\ +throttling.bps-read-max=6000,throttling.bps-write-max=6500,\ +throttling.iops-read-max=7000,throttling.iops-write-max=7500,\ +throttling.iops-size=2000,throttling.bps-read-max-length=3,\ +throttling.bps-write-max-length=5,throttling.iops-read-max-length=7,\ +throttling.iops-write-max-length=9 \ +-device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7080484..de74b69 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1492,6 +1492,10 @@ mymain(void) DO_TEST("blkdeviotune-max", QEMU_CAPS_DRIVE_IOTUNE, QEMU_CAPS_DRIVE_IOTUNE_MAX); + DO_TEST("blkdeviotune-max-length", + QEMU_CAPS_DRIVE_IOTUNE, + QEMU_CAPS_DRIVE_IOTUNE_MAX, + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH); DO_TEST("multifunction-pci-device", QEMU_CAPS_NODEFCONFIG, -- 2.7.4
On 23.09.2016 14:56, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1349898
Do a little housekeeping and minor adjustments to existing code, then add the various "-length" options for the <iotune> code.
John Ferlan (12): docs: Fix typo in libvirt-domain.h parameter description include: Update description for <iotune> max params tests: Add blkdeviotune-max xml2xmltest qemu: Convert from shorthand to longer throttling names qemu: Adjust how supportMaxOptions is used. include: Add new definitions for duration for bps/iops throttling caps: Add new capability for the bps/iops throttling length qemu: Add length for bps/iops throttling parameters to driver conf: Add a formatting macro for all the blkiotune values conf: Adjust the PARSE_IOTUNE macro conf: Add support for blkiotune "_length" options qemu: Add the length options to the iotune command line
docs/formatdomain.html.in | 40 +++++- docs/schemas/domaincommon.rng | 38 ++++++ include/libvirt/libvirt-domain.h | 124 ++++++++++++++++-- src/conf/domain_conf.c | 142 +++++++++++---------- src/conf/domain_conf.h | 6 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 96 ++++++-------- src/qemu/qemu_driver.c | 106 ++++++++++++++- src/qemu/qemu_monitor.c | 7 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 72 ++++++----- src/qemu/qemu_monitor_json.h | 3 +- .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 17 ++- .../qemuxml2argv-blkdeviotune-max-length.args | 34 +++++ .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++ .../qemuxml2argv-blkdeviotune-max.args | 10 +- .../qemuxml2argv-blkdeviotune-max.xml | 14 +- .../qemuxml2argv-blkdeviotune.args | 5 +- tests/qemuxml2argvtest.c | 4 + .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + .../qemuxml2xmlout-blkdeviotune-max.xml | 1 + tests/qemuxml2xmltest.c | 2 + 28 files changed, 616 insertions(+), 182 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml
ACK series. Michal
On 09/27/2016 11:21 AM, Michal Privoznik wrote:
On 23.09.2016 14:56, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1349898
Do a little housekeeping and minor adjustments to existing code, then add the various "-length" options for the <iotune> code.
John Ferlan (12): docs: Fix typo in libvirt-domain.h parameter description include: Update description for <iotune> max params tests: Add blkdeviotune-max xml2xmltest qemu: Convert from shorthand to longer throttling names qemu: Adjust how supportMaxOptions is used. include: Add new definitions for duration for bps/iops throttling caps: Add new capability for the bps/iops throttling length qemu: Add length for bps/iops throttling parameters to driver conf: Add a formatting macro for all the blkiotune values conf: Adjust the PARSE_IOTUNE macro conf: Add support for blkiotune "_length" options qemu: Add the length options to the iotune command line
[...]
ACK series.
Michal
Thanks - I pushed patches 1-5 and 9 I'm holding onto patches 6 & 7 until I can get the libvirt-perl adjustments ready (and libvirt-perl 2.3.0 is bottled up). I need to adjust patch 8 based on the virsh work - it's not major, just a "difference" in how the _length parameters can be set compared to how other values are set. I dropped patch 10 Once patch 8 is worked out, patches 11 & 12 haven't changed. And of course, the v2 will have the virsh command adjustments, plus a patch to qemu_driver.c as has been discussed as 8.5 in this series. John
On 09/23/2016 08:56 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1349898
Do a little housekeeping and minor adjustments to existing code, then add the various "-length" options for the <iotune> code.
I realized while working on code adding another <iotune> attribute that I neglected to update/test 'virsh blkdeviotune $dom $device'. I had checked that 'virsh dumpxml $dom' displayed the new values, but brain cramped on blkdeviotune... In any case, a simple enough task one would think, right? Well as it turns out, no so simple. First off with these patches applied, I got the following: # virsh blkdeviotune $dom $disk error: Unable to get block I/O throttle parameters error: internal error: nparams too large # Uh oh... Long story short, there are now 19 parameters, but only 16 are allowed as a maximum (from src/remote/remote_protocol.x): const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16; One would think "all" that would need to be done is change that 16 to say 24 (or 32 for more room). That led to a similar error.... Strange, until I discovered that the virTypedParamsDeserialize call in remoteDomainGetBlockIoTune used the wrong constant. So I changed that and fortunately my virsh works now - displaying what I want. BUT, again simple enough, right? Well not so fast. If I run an "older" virsh talking to the newer libvirtd, I get: # virsh blkdeviotune $dom $disk error: Unable to get block I/O throttle parameters error: Unable to decode message payload # Running that older virsh in a debugger, the failure occurs in xdr_remote_domain_get_block_io_tune_ret as soon as the 17th element is returned. I guess I'd expect that considering it's expecting at most 16 elements in it's xdr_array call. I guess part of me didn't expect this - the virsh client was able to allocate space for all 19 elements (due to the 0 nparams call being done first). I can understand the size of the returned data buffer not changing - that's not good, but if the client has done the right thing, then shouldn't that max param in the xdr_array call be the size of the buffer the client expects rather than the constant max? At least for the *Get* interfaces? So before I spent too much more time on digging on this - is this expected? (hopefully I've explained it well enough). Does this parameter count have to stay constant forever? It's too bad we didn't have the future vision that said there'd be more than 16 returned values. At this point I'm not sure what could be done from the (old) client perspective. If getting the old client error is OK or expected, then I can just go with the 24 or 32 for a BLOCK_IO_TUNE value. But I figured I'd put this out there and guage feedback. Tks - John FWIW: python client has similar phenomena
John Ferlan (12): docs: Fix typo in libvirt-domain.h parameter description include: Update description for <iotune> max params tests: Add blkdeviotune-max xml2xmltest qemu: Convert from shorthand to longer throttling names qemu: Adjust how supportMaxOptions is used. include: Add new definitions for duration for bps/iops throttling caps: Add new capability for the bps/iops throttling length qemu: Add length for bps/iops throttling parameters to driver conf: Add a formatting macro for all the blkiotune values conf: Adjust the PARSE_IOTUNE macro conf: Add support for blkiotune "_length" options qemu: Add the length options to the iotune command line
docs/formatdomain.html.in | 40 +++++- docs/schemas/domaincommon.rng | 38 ++++++ include/libvirt/libvirt-domain.h | 124 ++++++++++++++++-- src/conf/domain_conf.c | 142 +++++++++++---------- src/conf/domain_conf.h | 6 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 96 ++++++-------- src/qemu/qemu_driver.c | 106 ++++++++++++++- src/qemu/qemu_monitor.c | 7 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 72 ++++++----- src/qemu/qemu_monitor_json.h | 3 +- .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 17 ++- .../qemuxml2argv-blkdeviotune-max-length.args | 34 +++++ .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++ .../qemuxml2argv-blkdeviotune-max.args | 10 +- .../qemuxml2argv-blkdeviotune-max.xml | 14 +- .../qemuxml2argv-blkdeviotune.args | 5 +- tests/qemuxml2argvtest.c | 4 + .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + .../qemuxml2xmlout-blkdeviotune-max.xml | 1 + tests/qemuxml2xmltest.c | 2 + 28 files changed, 616 insertions(+), 182 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml
participants (3)
-
Erik Skultety -
John Ferlan -
Michal Privoznik