Re: [libvirt] [PATCH v2 09/12] Add host cache information into capabilities
On Wed, Apr 05, 2017 at 04:36:32PM +0200, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander
> ---
> 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, "\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,
> +
Re: [libvirt] [PATCH v2 09/12] Add host cache information into capabilities
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
signature.asc
Description: Digital signature
--
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
> +
> +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
[libvirt] [PATCH v2 09/12] Add host cache information into capabilities
Signed-off-by: Martin Kletzander
---
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
#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, "\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,
+ "\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, "\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, "\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;
+
+
