On Tue, Nov 14, 2017 at 07:38:50AM -0500, John Ferlan wrote:
On 11/13/2017 03:50 AM, Martin Kletzander wrote:Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> --- src/conf/capabilities.c | 45 ++++++++++++++-------- tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +- .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 4 +- .../vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml | 4 +- tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 4 +- 5 files changed, 36 insertions(+), 23 deletions(-)Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> John couple of noisy review thoughts below...diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 095ef51c424a..5bf8ac2019f9 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -883,7 +883,8 @@ virCapabilitiesFormatCaches(virBufferPtr buf, for (i = 0; i < ncaches; i++) { virCapsHostCacheBankPtr bank = caches[i]; char *cpus_str = virBitmapFormat(bank->cpus); - bool kilos = !(bank->size % 1024); + const char *unit = NULL; + unsigned long long short_size = virPrettySize(bank->size, &unit); if (!cpus_str) return -1; @@ -897,32 +898,44 @@ virCapabilitiesFormatCaches(virBufferPtr buf, "size='%llu' unit='%s' cpus='%s'", bank->id, bank->level, virCacheTypeToString(bank->type), - bank->size >> (kilos * 10), - kilos ? "KiB" : "B", - cpus_str); + short_size, unit, cpus_str); VIR_FREE(cpus_str); virBufferSetChildIndent(&controlBuf, buf); for (j = 0; j < bank->ncontrols; j++) {You could have "virResctrlInfoPtr controls = bank->controls[j];"
done
- bool min_kilos = !(bank->controls[j]->granularity % 1024); - - /* Only use KiB if both values are divisible */ - if (bank->controls[j]->min) - min_kilos = min_kilos && !(bank->controls[j]->min % 1024); + const char *min_unit; + unsigned long long gran_short_size = bank->controls[j]->granularity; + unsigned long long min_short_size = bank->controls[j]->min; + + gran_short_size = virPrettySize(gran_short_size, &unit); + min_short_size = virPrettySize(min_short_size, &min_unit); + + /* Only use the smaller unit if they are different */Or "if (STRNEQ(unit, min_unit))" - to be more faithful to the comment! I read this as - if min_short_size is there, then we check the unit by knowing the math to get the value. To some degree if the pretty format function allowed one to "choose" a specific format size that'd perhaps work too, but that's perhaps more work than it's worth.
if I add STRNEQ there it doesn't allow me to remove any code, it would just add a line. I need the math after that anyway.
+ if (min_short_size) { + unsigned long long gran_div; + unsigned long long min_div; + + gran_div = bank->controls[j]->granularity / gran_short_size; + min_div = bank->controls[j]->min / min_short_size; + + if (min_div > gran_div) { + min_short_size *= min_div / gran_div; + } else if (min_div < gran_div) { + unit = min_unit; + gran_short_size *= gran_div / min_div; + } + } virBufferAsprintf(&controlBuf, "<control granularity='%llu'", - bank->controls[j]->granularity >> (min_kilos * 10)); + gran_short_size); - if (bank->controls[j]->min) { - virBufferAsprintf(&controlBuf, - " min='%llu'", - bank->controls[j]->min >> (min_kilos * 10)); - } + if (min_short_size) + virBufferAsprintf(&controlBuf, " min='%llu'", min_short_size); virBufferAsprintf(&controlBuf, " unit='%s' type='%s' maxAllocs='%u'/>\n", - min_kilos ? "KiB" : "B", + unit, virCacheTypeToString(bank->controls[j]->scope), bank->controls[j]->max_allocation); }[...]
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
