Re: [PATCH] Expose resource control capabilites on cache bank

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

 

On Thu, Apr 06, 2017 at 06:56:09PM +0800, Eli Qiao wrote:



On Thursday, 6 April 2017 at 5:49 PM, Martin Kletzander wrote:


On Thu, Apr 06, 2017 at 04:20:06PM +0800, Eli Qiao wrote:
> This patch is based on Martin's cache branch.
>
> This patch amends the cache bank capability as follow:
>
> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
> <control min='768' unit='KiB' type='unified' nclos='4'/>
> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
> <control min='768' unit='KiB' type='unified' nclos='4'/>
>


Were we exposing the number of CLoS IDs before? Was there a discussion
about it? Do we want to expose them? Probably yes, I'm just wondering.



Just saw Daniel’s comments, we may need it, with ‘nallocations’, is that okay?

Do you want me to do a reversion now?




> Along with vircaps2xmltest case updated.
>
> Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx (mailto:liyong.qiao@xxxxxxxxx)>
> ---
> src/conf/capabilities.c | 112 +++++++++++++++++++++--
> src/conf/capabilities.h | 13 ++-
> tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +
> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 2 +
> tests/vircaps2xmltest.c | 11 +++
> 5 files changed, 132 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 416dd1a..75c0bec 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;
> @@ -900,6 +902,18 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> bank->size >> (kilos * 10),
> kilos ? "KiB" : "B",
> cpus_str);
> + 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' nclos='%u'/>\n",
> + bank->controls[j]->min >> (min_kilos * 10),
> + min_kilos ? "KiB" : "B",
> + virCacheTypeToString(bank->controls[j]->type),
> + bank->controls[j]->nclos);
> + }
> + virBufferAdjustIndent(buf, -2);
>
> VIR_FREE(cpus_str);
> }
> @@ -1516,10 +1530,93 @@ virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> if (!ptr)
> return;
>
> + size_t i;

Upstream frowns upon C99 initialization in the middle of the code.


Okay, I can more it before 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->nclos,
> + 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;
>


The return value is not used anywhere.

yes, I think we can ignore this value or not? if we enabled resctrl (we can always read correct thing from /sys/fs/resctrl/info/…)




We need to make sure we don't continue and report wrong information.
Like when VIR_APPEND_ELEMENT fails, you must report an error and fail
loading.


> +}
> +
> int
> virCapabilitiesInitCaches(virCapsPtr caps)
> {
> @@ -1533,11 +1630,6 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> 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;
> -
>


You are removing this and only reporting it for those we can control.
But I believe the cache information can be valuable by itself, even
for those who can't change it. If this was intentional, it should be
mentioned in the commit message.


Okay, I thought it was a temporary work around before we get resctrl information.

I can add it back if you think this is useful.




Maybe if level >= cache_min_level || bank->control (or something like that)


> /* offline CPUs don't provide cache info */
> if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
> return -1;
> @@ -1595,12 +1687,19 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> pos, ent->d_name) < 0)
> goto cleanup;
>
> - if (bank->level < cache_min_level) {
> + if ((ret = virCapabilitiesGetCacheControlType(bank)) < 0) {
> virCapsHostCacheBankFree(bank);
> bank = NULL;
> continue;
> }
>
> + if (ret == 0) {
> + virCapabilitiesGetCacheControl(bank, "");
> + } else if (ret == 1) {
> + virCapabilitiesGetCacheControl(bank, "CODE");
> + virCapabilitiesGetCacheControl(bank, "DATA");
> + }
> +
> for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
> *tmp_c = c_tolower(*tmp_c);
>
> @@ -1617,6 +1716,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
> break;
> }
> +
> if (i == caps->host.ncaches) {
> if (VIR_APPEND_ELEMENT(caps->host.caches,
> caps->host.ncaches,
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index e099ccc..13effdd 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 nclos; /* number class of id */
> +};
> 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;
> + virCapsHostCacheControlPtr *controls;
> };
>
> typedef struct _virCapsHost virCapsHost;
> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> index f2da28e..61269ea 100644
> --- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> +++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
> @@ -30,6 +30,8 @@
> </topology>
> <cache>
> <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/>
> + <control min='419430' unit='B' type='instruction' nclos='8'/>
> + <control min='419430' unit='B' type='data' nclos='8'/>
>


This file should have no <control/> in it. There is no resctrl for
these. I don't see the bug immeadiatelly, though.


Yes, I know the reason now,
On my local test host, I have enabled resctrl with CDP, but in the test case, we don’t mock /sys/fs/resctrl,
so it pick the real one.

I will remove it.

But the solution maybe we alway mock /sys/fs/resctrl to avoid use host’s default one?

is that okay?




sure, we need to mock it always.  Tests should not touch any part of the system.


> </cache>
> </host>
>
> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> index c30ea87..df27b94 100644
> --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
> @@ -42,7 +42,9 @@
> </topology>
> <cache>
> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/>
> + <control min='768' unit='KiB' type='unified' nclos='4'/>
> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/>
> + <control min='768' unit='KiB' type='unified' nclos='4'/>


As Dan said as well, this must be:
<bank>
 <cache/>
</control>


> </cache>
> </host>
>
> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> index f590249..93f776a 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;
> int ret = -1;
>
> /*
> @@ -58,6 +59,15 @@ test_virCapabilities(const void *opaque)
> data->resctrl ? "/system" : "") < 0)
> goto cleanup;
>
> + if (data->resctrl) {
> + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s%s",
>


just /resctrl instead of the last %s


Ah ha, stupid me, I get it now.


> + abs_srcdir, data->filename,
> + "/resctrl") < 0)
> + goto cleanup;
> + virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir);
> + }
> +
> +
> virFileMockAddPrefix("/sys/devices/system", dir);
> caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>
> @@ -84,6 +94,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
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx (mailto:libvir-list@xxxxxxxxxx)
> https://www.redhat.com/mailman/listinfo/libvir-list
>


--
libvir-list mailing list
libvir-list@xxxxxxxxxx (mailto:libvir-list@xxxxxxxxxx)
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux