[libvirt] [PATCH v2 09/12] Add host cache information into capabilities

Martin Kletzander posted 12 patches 8 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v2 09/12] Add host cache information into capabilities
Posted by Martin Kletzander 8 years, 10 months ago
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
I'm adding only info about L3 caches, we can add more later, but for
now that's more than enough.

 src/conf/capabilities.c                         | 200 ++++++++++++++++++++++++
 src/conf/capabilities.h                         |  29 ++++
 src/libvirt_private.syms                        |   1 +
 tests/vircaps2xmldata/vircaps-x86_64-caches.xml |   3 +
 tests/vircaps2xmltest.c                         |   3 +-
 5 files changed, 235 insertions(+), 1 deletion(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 7ed76e65b1a1..416dd1a34aba 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -31,6 +31,7 @@
 #include <unistd.h>

 #include "capabilities.h"
+#include "c-ctype.h"
 #include "count-one-bits.h"
 #include "cpu_conf.h"
 #include "domain_conf.h"
@@ -50,6 +51,8 @@

 #define VIR_FROM_THIS VIR_FROM_CAPABILITIES

+#define SYSFS_SYSTEM_PATH "/sys/devices/system/"
+
 VIR_LOG_INIT("conf.capabilities")

 VIR_ENUM_DECL(virCapsHostPMTarget)
@@ -237,6 +240,10 @@ virCapabilitiesDispose(void *object)
         virCapabilitiesClearSecModel(&caps->host.secModels[i]);
     VIR_FREE(caps->host.secModels);

+    for (i = 0; i < caps->host.ncaches; i++)
+        virCapsHostCacheBankFree(caps->host.caches[i]);
+    VIR_FREE(caps->host.caches);
+
     VIR_FREE(caps->host.netprefix);
     VIR_FREE(caps->host.pagesSize);
     virCPUDefFree(caps->host.cpu);
@@ -860,6 +867,49 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
     return 0;
 }

+static int
+virCapabilitiesFormatCaches(virBufferPtr buf,
+                            size_t ncaches,
+                            virCapsHostCacheBankPtr *caches)
+{
+    size_t i = 0;
+
+    if (!ncaches)
+        return 0;
+
+    virBufferAddLit(buf, "<cache>\n");
+    virBufferAdjustIndent(buf, 2);
+
+    for (i = 0; i < ncaches; i++) {
+        virCapsHostCacheBankPtr bank = caches[i];
+        char *cpus_str = virBitmapFormat(bank->cpus);
+        bool kilos = !(bank->size % 1024);
+
+        if (!cpus_str)
+            return -1;
+
+        /*
+         * Let's just *hope* the size is aligned to KiBs so that it does not
+         * bite is back in the future
+         */
+        virBufferAsprintf(buf,
+                          "<bank id='%u' level='%u' type='%s' "
+                          "size='%llu' unit='%s' cpus='%s'/>\n",
+                          bank->id, bank->level,
+                          virCacheTypeToString(bank->type),
+                          bank->size >> (kilos * 10),
+                          kilos ? "KiB" : "B",
+                          cpus_str);
+
+        VIR_FREE(cpus_str);
+    }
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</cache>\n");
+
+    return 0;
+}
+
 /**
  * virCapabilitiesFormatXML:
  * @caps: capabilities to format
@@ -956,6 +1006,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
                                           caps->host.numaCell) < 0)
         goto error;

+    if (virCapabilitiesFormatCaches(&buf, caps->host.ncaches,
+                                    caps->host.caches) < 0)
+        goto error;
+
     for (i = 0; i < caps->host.nsecModels; i++) {
         virBufferAddLit(&buf, "<secmodel>\n");
         virBufferAdjustIndent(&buf, 2);
@@ -1438,3 +1492,149 @@ virCapabilitiesInitPages(virCapsPtr caps)
     VIR_FREE(pages_size);
     return ret;
 }
+
+
+VIR_ENUM_IMPL(virCache, VIR_CACHE_TYPE_LAST,
+              "unified",
+              "instruction",
+              "data")
+
+bool
+virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
+                           virCapsHostCacheBankPtr b)
+{
+    return (a->id == b->id &&
+            a->level == b->level &&
+            a->type == b->type &&
+            a->size == b->size &&
+            virBitmapEqual(a->cpus, b->cpus));
+}
+
+void
+virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
+{
+    if (!ptr)
+        return;
+
+    virBitmapFree(ptr->cpus);
+    VIR_FREE(ptr);
+}
+
+int
+virCapabilitiesInitCaches(virCapsPtr caps)
+{
+    size_t i = 0;
+    virBitmapPtr cpus = NULL;
+    ssize_t pos = -1;
+    DIR *dirp = NULL;
+    int ret = -1;
+    char *path = NULL;
+    char *type = NULL;
+    struct dirent *ent = NULL;
+    virCapsHostCacheBankPtr bank = NULL;
+
+    /* Minimum level to expose in capabilities.  Can be lowered or removed (with
+     * the appropriate code below), but should not be increased, because we'd
+     * lose information. */
+    const int cache_min_level = 3;
+
+    /* offline CPUs don't provide cache info */
+    if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
+        return -1;
+
+    while ((pos = virBitmapNextSetBit(cpus, pos)) >= 0) {
+        int rv = -1;
+
+        VIR_FREE(path);
+        if (virAsprintf(&path, SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/", pos) < 0)
+            goto cleanup;
+
+        rv = virDirOpenIfExists(&dirp, path);
+        if (rv < 0)
+            goto cleanup;
+
+        if (!dirp)
+            continue;
+
+        while ((rv = virDirRead(dirp, &ent, path)) > 0) {
+            int tmp_i;
+            char *tmp_c;
+
+            if (!STRPREFIX(ent->d_name, "index"))
+                continue;
+
+            if (VIR_ALLOC(bank) < 0)
+                goto cleanup;
+
+            if (virFileReadValueUint(&bank->id,
+                                     SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/%s/id",
+                                     pos, ent->d_name) < 0)
+                goto cleanup;
+
+            if (virFileReadValueUint(&bank->level,
+                                     SYSFS_SYSTEM_PATH
+                                     "cpu/cpu%zd/cache/%s/level",
+                                     pos, ent->d_name) < 0)
+                goto cleanup;
+
+            if (virFileReadValueString(&type,
+                                       SYSFS_SYSTEM_PATH
+                                       "cpu/cpu%zd/cache/%s/type",
+                                       pos, ent->d_name) < 0)
+                goto cleanup;
+
+            if (virFileReadValueScaledInt(&bank->size,
+                                          SYSFS_SYSTEM_PATH
+                                          "cpu/cpu%zd/cache/%s/size",
+                                          pos, ent->d_name) < 0)
+                goto cleanup;
+
+            if (virFileReadValueBitmap(&bank->cpus,
+                                       SYSFS_SYSTEM_PATH
+                                       "cpu/cpu%zd/cache/%s/shared_cpu_list",
+                                       pos, ent->d_name) < 0)
+                goto cleanup;
+
+            if (bank->level < cache_min_level) {
+                virCapsHostCacheBankFree(bank);
+                bank = NULL;
+                continue;
+            }
+
+            for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
+                *tmp_c = c_tolower(*tmp_c);
+
+            tmp_i = virCacheTypeFromString(type);
+            if (tmp_i < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Unknown cache type '%s'"), type);
+                VIR_FREE(type);
+                goto cleanup;
+            }
+            bank->type = tmp_i;
+
+            for (i = 0; i < caps->host.ncaches; i++) {
+                if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
+                    break;
+            }
+            if (i == caps->host.ncaches) {
+                if (VIR_APPEND_ELEMENT(caps->host.caches,
+                                       caps->host.ncaches,
+                                       bank) < 0) {
+                    goto cleanup;
+                }
+            }
+
+            virCapsHostCacheBankFree(bank);
+        }
+        if (rv < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(path);
+    virDirClose(&dirp);
+    virCapsHostCacheBankFree(bank);
+    return ret;
+}
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index d10eef3afdea..e099cccf6d39 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -138,6 +138,26 @@ struct _virCapsHostSecModel {
     virCapsHostSecModelLabelPtr labels;
 };

+typedef enum {
+    VIR_CACHE_TYPE_DATA,
+    VIR_CACHE_TYPE_INSTRUCTION,
+    VIR_CACHE_TYPE_UNIFIED,
+
+    VIR_CACHE_TYPE_LAST
+} virCacheType;
+
+VIR_ENUM_DECL(virCache);
+
+typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
+typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;
+struct _virCapsHostCacheBank {
+    unsigned int id;
+    unsigned int level; /* 1=L1, 2=L2, 3=L3, etc. */
+    unsigned long long size; /* B */
+    virCacheType type;  /* Data, Instruction or Unified */
+    virBitmapPtr cpus;  /* All CPUs that share this bank */
+};
+
 typedef struct _virCapsHost virCapsHost;
 typedef virCapsHost *virCapsHostPtr;
 struct _virCapsHost {
@@ -157,6 +177,9 @@ struct _virCapsHost {
     size_t nnumaCell_max;
     virCapsHostNUMACellPtr *numaCell;

+    size_t ncaches;
+    virCapsHostCacheBankPtr *caches;
+
     size_t nsecModels;
     virCapsHostSecModelPtr secModels;

@@ -303,4 +326,10 @@ int virCapabilitiesInitPages(virCapsPtr caps);

 int virCapabilitiesInitNUMA(virCapsPtr caps);

+bool virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
+                                virCapsHostCacheBankPtr b);
+void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr);
+
+int virCapabilitiesInitCaches(virCapsPtr caps);
+
 #endif /* __VIR_CAPABILITIES_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d399e0dc063a..b175b3b02901 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -59,6 +59,7 @@ virCapabilitiesFreeNUMAInfo;
 virCapabilitiesGetCpusForNodemask;
 virCapabilitiesGetNodeInfo;
 virCapabilitiesHostSecModelAddBaseLabel;
+virCapabilitiesInitCaches;
 virCapabilitiesInitNUMA;
 virCapabilitiesInitPages;
 virCapabilitiesNew;
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
index 88f2ec62277e..f2da28e576ac 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
@@ -28,6 +28,9 @@
         </cell>
       </cells>
     </topology>
+    <cache>
+      <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/>
+    </cache>
   </host>

 </capabilities>
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index af422238520b..8f02314afe8e 100644
--- a/tests/vircaps2xmltest.c
+++ b/tests/vircaps2xmltest.c
@@ -58,7 +58,8 @@ test_virCapabilities(const void *opaque)
     if (!caps)
         goto cleanup;

-    if (virCapabilitiesInitNUMA(caps) < 0)
+    if (virCapabilitiesInitNUMA(caps) < 0 ||
+        virCapabilitiesInitCaches(caps) < 0)
         goto cleanup;

     virFileMockClearPrefixes();
--
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/12] Add host cache information into capabilities
Posted by Eli Qiao 8 years, 10 months ago
> +
> +VIR_ENUM_IMPL(virCache, VIR_CACHE_TYPE_LAST,
> + "unified",
> + "instruction",
> + "data")
> +


> };
> 
> +typedef enum {
> + VIR_CACHE_TYPE_DATA,
> + VIR_CACHE_TYPE_INSTRUCTION,
> + VIR_CACHE_TYPE_UNIFIED,
> +
> + VIR_CACHE_TYPE_LAST
> +} virCacheType;
> +

The sequence is wrong, it should be :

VIR_CACHE_TYPE_UNIFIED,
VIR_CACHE_TYPE_INSTRUCTION,
VIR_CACHE_TYPE_DATA,


see the above VIR_ENUM_IMPL.
> +VIR_ENUM_DECL(virCache);
> +
> +typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
> 
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/12] Add host cache information into capabilities
Posted by Martin Kletzander 8 years, 10 months ago
On Thu, Apr 06, 2017 at 02:04:04PM +0800, Eli Qiao wrote:
>
>> +
>> +VIR_ENUM_IMPL(virCache, VIR_CACHE_TYPE_LAST,
>> + "unified",
>> + "instruction",
>> + "data")
>> +
>
>
>> };
>>
>> +typedef enum {
>> + VIR_CACHE_TYPE_DATA,
>> + VIR_CACHE_TYPE_INSTRUCTION,
>> + VIR_CACHE_TYPE_UNIFIED,
>> +
>> + VIR_CACHE_TYPE_LAST
>> +} virCacheType;
>> +
>
>The sequence is wrong, it should be :
>
>VIR_CACHE_TYPE_UNIFIED,
>VIR_CACHE_TYPE_INSTRUCTION,
>VIR_CACHE_TYPE_DATA,
>

Hehe, good catch.  Since the internal values are not exposed anywhere,
the test data aren't affected.  I wish I remembered why I changed that.

For future reviewers, consider the following squashed in:

diff --git i/src/conf/capabilities.h w/src/conf/capabilities.h
index e099cccf6d39..60c8218ce3e5 100644
--- i/src/conf/capabilities.h
+++ w/src/conf/capabilities.h
@@ -139,9 +139,9 @@ struct _virCapsHostSecModel {
 };

 typedef enum {
-    VIR_CACHE_TYPE_DATA,
-    VIR_CACHE_TYPE_INSTRUCTION,
     VIR_CACHE_TYPE_UNIFIED,
+    VIR_CACHE_TYPE_INSTRUCTION,
+    VIR_CACHE_TYPE_DATA,

     VIR_CACHE_TYPE_LAST
 } virCacheType;
--

Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/12] Add host cache information into capabilities
Posted by Erik Skultety 8 years, 10 months ago
On Wed, Apr 05, 2017 at 04:36:32PM +0200, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> I'm adding only info about L3 caches, we can add more later, but for
> now that's more than enough.

I think it's worth putting this into the commit message, along with a snippet
introducing the XML update.

[...]

> +static int
> +virCapabilitiesFormatCaches(virBufferPtr buf,
> +                            size_t ncaches,
> +                            virCapsHostCacheBankPtr *caches)
> +{
> +    size_t i = 0;
> +
> +    if (!ncaches)
> +        return 0;
> +
> +    virBufferAddLit(buf, "<cache>\n");
> +    virBufferAdjustIndent(buf, 2);
> +
> +    for (i = 0; i < ncaches; i++) {
> +        virCapsHostCacheBankPtr bank = caches[i];
> +        char *cpus_str = virBitmapFormat(bank->cpus);
> +        bool kilos = !(bank->size % 1024);

I'd suggest kibs/kibis/kibi but not kilos.

> +
> +int
> +virCapabilitiesInitCaches(virCapsPtr caps)
> +{
> +    size_t i = 0;
> +    virBitmapPtr cpus = NULL;
> +    ssize_t pos = -1;
> +    DIR *dirp = NULL;
> +    int ret = -1;
> +    char *path = NULL;
> +    char *type = NULL;
> +    struct dirent *ent = NULL;
> +    virCapsHostCacheBankPtr bank = NULL;
> +
> +    /* Minimum level to expose in capabilities.  Can be lowered or removed (with
> +     * the appropriate code below), but should not be increased, because we'd
> +     * lose information. */
> +    const int cache_min_level = 3;
> +
> +    /* offline CPUs don't provide cache info */
> +    if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
> +        return -1;
> +
> +    while ((pos = virBitmapNextSetBit(cpus, pos)) >= 0) {
> +        int rv = -1;
> +
> +        VIR_FREE(path);
> +        if (virAsprintf(&path, SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/", pos) < 0)
> +            goto cleanup;
> +
> +        rv = virDirOpenIfExists(&dirp, path);
> +        if (rv < 0)
> +            goto cleanup;
> +
> +        if (!dirp)
> +            continue;
> +
> +        while ((rv = virDirRead(dirp, &ent, path)) > 0) {
> +            int tmp_i;
> +            char *tmp_c;
> +
> +            if (!STRPREFIX(ent->d_name, "index"))
> +                continue;
> +
> +            if (VIR_ALLOC(bank) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueUint(&bank->id,
> +                                     SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/%s/id",
> +                                     pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueUint(&bank->level,
> +                                     SYSFS_SYSTEM_PATH
> +                                     "cpu/cpu%zd/cache/%s/level",
> +                                     pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueString(&type,
> +                                       SYSFS_SYSTEM_PATH
> +                                       "cpu/cpu%zd/cache/%s/type",
> +                                       pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueScaledInt(&bank->size,
> +                                          SYSFS_SYSTEM_PATH
> +                                          "cpu/cpu%zd/cache/%s/size",
> +                                          pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueBitmap(&bank->cpus,
> +                                       SYSFS_SYSTEM_PATH
> +                                       "cpu/cpu%zd/cache/%s/shared_cpu_list",
> +                                       pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (bank->level < cache_min_level) {
> +                virCapsHostCacheBankFree(bank);
> +                bank = NULL;
> +                continue;
> +            }

I'd say, make the bank->level read first, then perform ^this check and then
continue with the rest of the checks...you know, to save a few cycles...

> +
> +            for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
> +                *tmp_c = c_tolower(*tmp_c);
> +
> +            tmp_i = virCacheTypeFromString(type);
> +            if (tmp_i < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Unknown cache type '%s'"), type);
> +                VIR_FREE(type);
> +                goto cleanup;
> +            }

I'd maybe add a small static function handling ^this hunk (or add a symmetric
virStringToLower method), but that's a bikeshed for another day.

> +            bank->type = tmp_i;
> +
> +            for (i = 0; i < caps->host.ncaches; i++) {
> +                if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
> +                    break;
> +            }
> +            if (i == caps->host.ncaches) {
> +                if (VIR_APPEND_ELEMENT(caps->host.caches,
> +                                       caps->host.ncaches,
> +                                       bank) < 0) {
> +                    goto cleanup;
> +                }
> +            }

I'd wrap ^this here as well, a small boolean (possibly inline) method called
virCapsHostCacheBankExists, if you get false, then add it to the list, but feel
free to disagree with me on this.

ACK if you enhance the commit message.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list