Re: [PATCH 08.5/12] qemu: Create a set of macros to handle setting block iotune values

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

 



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@xxxxxxxxxx>
>>> ---
>>>
>>>  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 = &params[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)) {
>>>
>>
>>


Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]