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

Re: [libvirt] [PATCH 11/21] conf: Format cache banks in capabilities with virPrettySize



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 redhat com>
---
 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 redhat com>

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


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