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

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(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, > 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);

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