[libvirt] [PATCH] Expose resource control capabilites on cache bank

Thu Apr 6 11:49:01 UTC 2017

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 at intel.com (mailto:liyong.qiao at intel.com)>
>> > ---
>> > 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 at redhat.com (mailto:libvir-list at redhat.com)
>> > https://www.redhat.com/mailman/listinfo/libvir-list
>> >
>>
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com (mailto:libvir-list at redhat.com)
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
>
>
-------------- 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/ac1fd406/attachment-0001.sig>