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

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