[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 14:57:29 UTC 2016
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 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.
>>
>
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
-------------- 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/921a8b7d/attachment-0001.sig>
More information about the libvir-list
mailing list