Re: [libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

[John Ferlan]

On 10/25/2016 01:30 PM, Erik Skultety wrote:
> On Thu, Oct 06, 2016 at 06:38:56PM -0400, 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 
>> ---
>>  include/libvirt/libvirt-domain.h |  54 +
>>  src/conf/domain_conf.h   |   6 +++
>>  src/qemu/qemu_driver.c   | 100 
>> +--
>>  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, 202 insertions(+), 13 deletions(-)
>>
> 
> [...]
> 
>> @@ -17296,7 +17297,9 @@ 
>> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
> 
> just a nitpick, are both the 'Set's necessary in the name, unless one of them
> is a verb and the other a noun, I think the first one could be dropped.
> 

D'oh... The name was so long I missed the double set!

>>  bool set_iops,
>>  bool set_bytes_max,
>>  bool set_iops_max,
>> -bool set_size_iops)
>> +bool set_size_iops,
>> +bool set_bytes_max_length,
>> +bool set_iops_max_length)
>>  {
>>  if (!set_bytes) {
>>  newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
>> @@ -17320,6 +17323,36 @@ 
>> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>>  }
>>  if (!set_size_iops)
>>  newinfo->size_iops_sec = oldinfo->size_iops_sec;
>> +
>> +/* The length field is handled a bit differently. If not defined/set,
>> + * QEMU will default these to 0 or 1 depending on whether something in
>> + * the same family is set or not.
>> + *
>> + * Similar to other values, if nothing in the family is defined/set,
>> + * then take whatever is in the oldinfo.
>> + *
>> + * To clear an existing limit, a 0 is provided; however, passing that
>> + * 0 onto QEMU if there's a family value defined/set (or defaulted)
>> + * will cause an error. So, to mimic that, if our oldinfo was set and
>> + * our newinfo is clearing, then set max_length based on whether we
>> + * have a value in the family set/defined. */
> 
> Hmm. You can disregard my comment in 4/12 about replacing the whole function
> with a macro, instead I suggest keeping the function and making the macro 
> below
> also work for all the settings above, otherwise it just doesn't look right, 
> one
> part of the function being expanded while the other covered by a macro because
> the behaviour is slightly different (I have to admit though, it was kinda
> painful to comprehend what's going on on the qemu side)
> 

Yes, quite painful - especially when I realized it after the initial
series...  The difference is I went with a function.

I create a macro for the setting of the defaults

>> +#define SET_MAX_LENGTH(BOOL, FIELD) 
>>\
>> +if (!BOOL)  
>>\
>> +newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length;  
>>\
>> +else if (BOOL && oldinfo->FIELD##_max_length && 
>>\
>> + !newinfo->FIELD##_max_length)  
>>\
>> +newinfo->FIELD##_max_length = (newinfo->FIELD ||
>>\
>> +   newinfo->FIELD##_max) ? 1 : 0;
>> +
>> +SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec);
>> +SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec);
>> +SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, total_iops_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, read_iops_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, write_iops_sec);
>> +
>> +#undef SET_MAX_LENGTH
>> +
>>  }
>>  
>>  
>> @@ -17346,7 +17379,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;
>>  

Re: [libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:56PM -0400, 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 
> ---
>  include/libvirt/libvirt-domain.h |  54 +
>  src/conf/domain_conf.h   |   6 +++
>  src/qemu/qemu_driver.c   | 100 
> +--
>  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, 202 insertions(+), 13 deletions(-)
> 

[...]

> @@ -17296,7 +17297,9 @@ 
> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,

just a nitpick, are both the 'Set's necessary in the name, unless one of them
is a verb and the other a noun, I think the first one could be dropped.

>  bool set_iops,
>  bool set_bytes_max,
>  bool set_iops_max,
> -bool set_size_iops)
> +bool set_size_iops,
> +bool set_bytes_max_length,
> +bool set_iops_max_length)
>  {
>  if (!set_bytes) {
>  newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
> @@ -17320,6 +17323,36 @@ 
> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>  }
>  if (!set_size_iops)
>  newinfo->size_iops_sec = oldinfo->size_iops_sec;
> +
> +/* The length field is handled a bit differently. If not defined/set,
> + * QEMU will default these to 0 or 1 depending on whether something in
> + * the same family is set or not.
> + *
> + * Similar to other values, if nothing in the family is defined/set,
> + * then take whatever is in the oldinfo.
> + *
> + * To clear an existing limit, a 0 is provided; however, passing that
> + * 0 onto QEMU if there's a family value defined/set (or defaulted)
> + * will cause an error. So, to mimic that, if our oldinfo was set and
> + * our newinfo is clearing, then set max_length based on whether we
> + * have a value in the family set/defined. */

Hmm. You can disregard my comment in 4/12 about replacing the whole function
with a macro, instead I suggest keeping the function and making the macro below
also work for all the settings above, otherwise it just doesn't look right, one
part of the function being expanded while the other covered by a macro because
the behaviour is slightly different (I have to admit though, it was kinda
painful to comprehend what's going on on the qemu side)

> +#define SET_MAX_LENGTH(BOOL, FIELD)  
>   \
> +if (!BOOL)   
>   \
> +newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length;   
>   \
> +else if (BOOL && oldinfo->FIELD##_max_length &&  
>   \
> + !newinfo->FIELD##_max_length)   
>   \
> +newinfo->FIELD##_max_length = (newinfo->FIELD || 
>   \
> +   newinfo->FIELD##_max) ? 1 : 0;
> +
> +SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec);
> +SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec);
> +SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec);
> +SET_MAX_LENGTH(set_iops_max_length, total_iops_sec);
> +SET_MAX_LENGTH(set_iops_max_length, read_iops_sec);
> +SET_MAX_LENGTH(set_iops_max_length, write_iops_sec);
> +
> +#undef SET_MAX_LENGTH
> +
>  }
>  
>  
> @@ -17346,7 +17379,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;
> @@ -17382,6 +17418,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,
> +