[libvirt] [PATCH 08.5/12] qemu: Create a set of macros to handle setting block iotune values
Erik Skultety
eskultet at redhat.com
Mon Oct 3 07:56:50 UTC 2016
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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: set_iotune_field.patch
Type: text/x-patch
Size: 13158 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161003/c2a709d8/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161003/c2a709d8/attachment-0001.sig>
More information about the libvir-list
mailing list