[libvirt] [PATCH V3] Expose resource control capabilites on cache bank
Martin Kletzander
mkletzan at redhat.com
Thu Apr 6 12:46:10 UTC 2017
On Thu, Apr 06, 2017 at 08:20:56PM +0800, Eli Qiao wrote:
>This patch is based on Martin's cache branch.
>
>This patch amends the cache bank capability as follow:
>
It helps a lot if you wait for a conclusion on a patch before sending
another version as soon as you can change one line.
><cache>
> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
> <control min='768' unit='KiB' type='unified' nallocations='4'/>
> </bank>
> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
> <control min='768' unit='KiB' type='unified' nallocations='4'/>
> </bank>
></cache>
>
I know Dan proposed "nallocations", but it sounds like one word. I
would rather use "allocations" or "max_allocs" or something
understandable. The reason for it? We have no documentation for our
capabilities XML. And nobody is trying to create one as far as I know.
So at least the naming should be more intuitive.
>Along with vircaps2xmltest case updated.
>
I'm sensing you haven't ran the tests. Am I right?
>Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
>---
> src/conf/capabilities.c | 120 ++++++++++++++++++++++-
> src/conf/capabilities.h | 13 ++-
> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +-
> tests/vircaps2xmltest.c | 13 +++
> 4 files changed, 148 insertions(+), 6 deletions(-)
>
>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>index 416dd1a..3094e48 100644
>--- a/src/conf/capabilities.c
>+++ b/src/conf/capabilities.c
>@@ -52,6 +52,7 @@
> #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
>
> #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
>+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
>
> VIR_LOG_INIT("conf.capabilities")
>
>@@ -873,6 +874,7 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> virCapsHostCacheBankPtr *caches)
> {
> size_t i = 0;
>+ size_t j = 0;
>
> if (!ncaches)
> return 0;
>@@ -894,13 +896,32 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> */
> virBufferAsprintf(buf,
> "<bank id='%u' level='%u' type='%s' "
>- "size='%llu' unit='%s' cpus='%s'/>\n",
>+ "size='%llu' unit='%s' cpus='%s'",
> bank->id, bank->level,
> virCacheTypeToString(bank->type),
> bank->size >> (kilos * 10),
> kilos ? "KiB" : "B",
> cpus_str);
>
>+ if (bank->ncontrol > 0) {
>+ virBufferAddLit(buf, ">\n");
>+ virBufferAdjustIndent(buf, 2);
>+ for (j = 0; j < bank->ncontrol; j++) {
>+ bool min_kilos = !(bank->controls[j]->min % 1024);
>+ virBufferAsprintf(buf,
>+ "<control min='%llu' unit='%s' "
>+ "type='%s' nallocations='%u'/>\n",
>+ bank->controls[j]->min >> (min_kilos * 10),
>+ min_kilos ? "KiB" : "B",
>+ virCacheTypeToString(bank->controls[j]->type),
>+ bank->controls[j]->nallocations);
>+ }
>+ virBufferAdjustIndent(buf, -2);
>+ virBufferAddLit(buf, "</bank>\n");
>+ } else {
>+ virBufferAddLit(buf, "/>\n");
>+ }
>+
There's virBufferAddBuffer() for easier usage of this. Just grep for
childrenBuf and you'll see the usage.
> VIR_FREE(cpus_str);
> }
>
>@@ -1513,13 +1534,97 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
> void
> virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> {
>+ size_t i;
>+
> if (!ptr)
> return;
>
> virBitmapFree(ptr->cpus);
>+ for (i = 0; i < ptr->ncontrol; i++)
>+ VIR_FREE(ptr->controls[i]);
>+ VIR_FREE(ptr->controls);
> VIR_FREE(ptr);
> }
>
>+/* test which kinds of cache control supported
>+ * -1: don't support
>+ * 0: cat
>+ * 1: cdp
>+ */
>+static int
>+virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
>+{
>+ int ret = -1;
>+ char *path = NULL;
>+ if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%u", bank->level) < 0)
>+ return -1;
>+
>+ if (virFileExists(path)) {
>+ ret = 0;
>+ } else {
>+ VIR_FREE(path);
>+ if (virAsprintf(&path, SYSFS_RESCTRL_PATH "info/L%uCODE", bank->level) < 0)
>+ return -1;
>+ if (virFileExists(path))
>+ ret = 1;
>+ }
>+
>+ VIR_FREE(path);
>+ return ret;
>+}
>+
>+static int
>+virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank, const char* type)
>+{
>+ int ret = -1;
>+ char *path = NULL;
>+ char *cbm_mask = NULL;
>+ virCapsHostCacheControlPtr control;
>+
>+ if (VIR_ALLOC(control) < 0)
>+ goto cleanup;
>+
>+ if (virFileReadValueUint(&control->nallocations,
>+ SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
>+ bank->level,
>+ type) < 0)
>+ goto cleanup;
>+
>+ if (virFileReadValueString(&cbm_mask,
>+ SYSFS_RESCTRL_PATH
>+ "info/L%u%s/cbm_mask",
>+ bank->level,
>+ type) < 0)
>+ goto cleanup;
>+
>+ virStringTrimOptionalNewline(cbm_mask);
>+
>+ control->min = bank->size / (strlen(cbm_mask) * 4);
>+
>+ if (STREQ("", type))
>+ control->type = VIR_CACHE_TYPE_UNIFIED;
>+ else if (STREQ("CODE", type))
>+ control->type = VIR_CACHE_TYPE_INSTRUCTION;
>+ else if (STREQ("DATA", type))
>+ control->type = VIR_CACHE_TYPE_DATA;
>+ else
>+ goto cleanup;
>+
>+ if (VIR_APPEND_ELEMENT(bank->controls,
>+ bank->ncontrol,
>+ control) < 0)
>+ goto error;
>+
>+ ret = 0;
>+
>+ cleanup:
>+ VIR_FREE(path);
>+ return ret;
>+ error:
>+ VIR_FREE(control);
>+ return -1;
>+}
>+
> int
> virCapabilitiesInitCaches(virCapsPtr caps)
> {
>@@ -1595,7 +1700,18 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> pos, ent->d_name) < 0)
> goto cleanup;
>
>- if (bank->level < cache_min_level) {
>+ ret = virCapabilitiesGetCacheControlType(bank);
>+
>+ if ((bank->level >= cache_min_level) || (ret >= 0)) {
>+ if (ret == 0) {
>+ if (virCapabilitiesGetCacheControl(bank, "") < 0)
>+ goto cleanup;
>+ } else if (ret == 1) {
>+ if ((virCapabilitiesGetCacheControl(bank, "CODE") < 0) ||
>+ (virCapabilitiesGetCacheControl(bank, "DATA") < 0))
>+ goto cleanup;
>+ }
>+ } else {
> virCapsHostCacheBankFree(bank);
> bank = NULL;
> continue;
>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
>index e099ccc..1007c30 100644
>--- a/src/conf/capabilities.h
>+++ b/src/conf/capabilities.h
>@@ -139,15 +139,22 @@ 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;
>
> VIR_ENUM_DECL(virCache);
>
>+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
>+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
>+struct _virCapsHostCacheControl {
>+ unsigned long long min; /* B */
>+ virCacheType type; /* Data, Instruction or Unified */
>+ unsigned int nallocations; /* number of supported allocation */
"number of supported allocations"
>+};
> typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
> typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;
> struct _virCapsHostCacheBank {
>@@ -156,6 +163,8 @@ struct _virCapsHostCacheBank {
> unsigned long long size; /* B */
> virCacheType type; /* Data, Instruction or Unified */
> virBitmapPtr cpus; /* All CPUs that share this bank */
>+ size_t ncontrol;
ncontrols. as I said, look at the code around, try reasoning about
differences if it's already inconsistent.
>+ virCapsHostCacheControlPtr *controls;
> };
>
> typedef struct _virCapsHost virCapsHost;
>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>index c30ea87..32a9549 100644
>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>@@ -41,8 +41,12 @@
> </cells>
> </topology>
> <cache>
>- <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
>- <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
>+ <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>+ <control min='768' unit='KiB' type='unified' nallocations='4'/>
>+ </bank>
>+ <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
>+ <control min='768' unit='KiB' type='unified' nallocations='4'/>
>+ </bank>
> </cache>
> </host>
>
You could add new test to also show how it looks on other machines. For
example how does this look like when CDP is enabled. Just copy related
directories and files (not all of them, just use common sense) from your
machine or some machine you have access to and it differs from what we
have in the tests already.
>diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
>index f590249..3d1ad43 100644
>--- a/tests/vircaps2xmltest.c
>+++ b/tests/vircaps2xmltest.c
>@@ -47,6 +47,7 @@ test_virCapabilities(const void *opaque)
> char *capsXML = NULL;
> char *path = NULL;
> char *dir = NULL;
>+ char *resctrl_dir = NULL;
in this case "dir" should be removed so the naming is more intuitive.
> int ret = -1;
>
> /*
>@@ -58,6 +59,17 @@ test_virCapabilities(const void *opaque)
> data->resctrl ? "/system" : "") < 0)
> goto cleanup;
>
>+ if (data->resctrl) {
>+ if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl",
>+ abs_srcdir, data->filename) < 0)
>+ goto cleanup;
>+ } else {
>+ /* Mock to bad directory to avoid using /sys/fs/resctrl */
>+ if (VIR_STRDUP(resctrl_dir, "") < 0)
>+ goto cleanup;
>+ }
>+
Why so complicated? Why not just doing the virAsprintf unconditionally?
>+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);
> virFileMockAddPrefix("/sys/devices/system", dir);
> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>
>@@ -84,6 +96,7 @@ test_virCapabilities(const void *opaque)
>
> cleanup:
> VIR_FREE(dir);
>+ VIR_FREE(resctrl_dir);
> VIR_FREE(path);
> VIR_FREE(capsXML);
> virObjectUnref(caps);
>--
>1.9.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170406/863c8255/attachment-0001.sig>
More information about the libvir-list
mailing list