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

Friday, 30 September 2016

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(a)redhat.com&gt;
 ---

  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 = &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)) {

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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