Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
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:
>
>
>
>
>
>
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 mailto:liyong.q...@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,
> + " + "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
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thu, Apr 06, 2017 at 07:31:00PM +0800, Eli Qiao wrote: > > > On Thursday, 6 April 2017 at 5:51 PM, Daniel P. Berrange wrote: > > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > > cpus='0-5'/> > > > > > > > > > > > cpus='6-11'/> > > > > > > > > > > > > > > > > > > > > Either the XML is malformed, or the indentation is wrong. The indentation > > suggests you want nested XML elements, but the parent element is an empty > > tag, so you've actually got a flat namespace here. > > > > > > > > > > > > I validate it online, no errors, the result is: You've checked the XML syntax, but *not* the semantics. You've implemented this: but indentation suggests you meant to do both are valid XML, but they have completely different semantics Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thursday, 6 April 2017 at 5:51 PM, Daniel P. Berrange wrote: > On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > > > > > cpus='6-11'/> > > > > > > > > > > > > > Either the XML is malformed, or the indentation is wrong. The indentation > suggests you want nested XML elements, but the parent element is an empty > tag, so you've actually got a flat namespace here. > I validate it online, no errors, the result is: http://www.xmlvalidation.com/index.php?id=1&L=0&target=/xmlvalidation/start.jsp;jsessionid=5B8746DEE269CE0ADFB1B8F88CF5E94F > > > > > > > > > > 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. > > > > > What are CLoS IDs and what are they used for ? > > > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| > > -- > libvir-list mailing list > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
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:
> >
> >
> >
> >
> >
> >
>
>
> 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 > (mailto:liyong.q...@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,
> > + " > + "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/…)
>
> > +}
> > +
> > 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
>
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thu, Apr 06, 2017 at 06:29:47PM +0800, Eli Qiao wrote: > > > On Thursday, 6 April 2017 at 6:10 PM, Daniel P. Berrange wrote: > > > On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote: > > > On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote: > > > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > > > > > > > > cpus='0-5'/> > > > > > > > > > > > > > > > > > cpus='6-11'/> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Either the XML is malformed, or the indentation is wrong. The > > > > indentation > > > > suggests you want nested XML elements, but the parent element is an > > > > empty > > > > tag, so you've actually got a flat namespace here. > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > What are CLoS IDs and what are they used for ? > > > > > > Effectively an ID for the allocation. The hardware has a limited number > > > of them, in this case 4. I can't remember whether that number is > > > per-bank, but it would not make much sense otherwise. > > > > > > > > > So, if guests are requesting a private cache allocation, and cos id == 4, > > then we can only run 4 guests ? > > > > > > > I think yes, but it’s per bank resource (the bank here is equal to cache > id), if you have 2 banks , you can create 8 guests (each has 1 bank > allocation) Ok, well if it is resource limiting in this way, then I think it is important to expose in the capabilities. This information would be needed by openstack schedular in order to avoid placing too guests on the same host when they have requested cache allocation. Picking a more obvious attr name would be nice though eg 'nallocations' Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
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: > > > > > > > > > > > > > > > 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. > > > We don’t expose it in previous discussion as in my old design, it is saved in a global internal struct, and we will use it to limit the cache allocation number. For now as you expose all these to capabilities XML, I want expose it out which can be reference by resctrl utils later. Thought? > > -- > > libvir-list mailing list > > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > > https://www.redhat.com/mailman/listinfo/libvir-list > > > > > -- > libvir-list mailing list > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thursday, 6 April 2017 at 6:10 PM, Daniel P. Berrange wrote: > On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote: > > On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote: > > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > > > > > cpus='0-5'/> > > > > > > > > > > > > > > cpus='6-11'/> > > > > > > > > > > > > > > > > > > > > > > > > > > > Either the XML is malformed, or the indentation is wrong. The indentation > > > suggests you want nested XML elements, but the parent element is an empty > > > tag, so you've actually got a flat namespace here. > > > > > > > > > > > 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. > > > > > > > > > > > > > What are CLoS IDs and what are they used for ? > > > > Effectively an ID for the allocation. The hardware has a limited number > > of them, in this case 4. I can't remember whether that number is > > per-bank, but it would not make much sense otherwise. > > > > > So, if guests are requesting a private cache allocation, and cos id == 4, > then we can only run 4 guests ? > > I think yes, but it’s per bank resource (the bank here is equal to cache id), if you have 2 banks , you can create 8 guests (each has 1 bank allocation) As far as I know, the number of clos id is 16 on most of Intel xeon CPUs > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| > > -- > libvir-list mailing list > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thu, Apr 06, 2017 at 12:05:54PM +0200, Martin Kletzander wrote: > On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote: > > On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > > cpus='0-5'/> > > > > > > > > > > > cpus='6-11'/> > > > > > > > > Either the XML is malformed, or the indentation is wrong. The indentation > > suggests you want nested XML elements, but the parent element is an empty > > tag, so you've actually got a flat namespace here. > > > > > > > > > > > > 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. > > > > What are CLoS IDs and what are they used for ? > > > > Effectively an ID for the allocation. The hardware has a limited number > of them, in this case 4. I can't remember whether that number is > per-bank, but it would not make much sense otherwise. So, if guests are requesting a private cache allocation, and cos id == 4, then we can only run 4 guests ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thu, Apr 06, 2017 at 10:51:44AM +0100, Daniel P. Berrange wrote: On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > Either the XML is malformed, or the indentation is wrong. The indentation suggests you want nested XML elements, but the parent element is an empty tag, so you've actually got a flat namespace here. > 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. What are CLoS IDs and what are they used for ? Effectively an ID for the allocation. The hardware has a limited number of them, in this case 4. I can't remember whether that number is per-bank, but it would not make much sense otherwise. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
On Thu, Apr 06, 2017 at 11:49:14AM +0200, 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: > > > > > > > > > > Either the XML is malformed, or the indentation is wrong. The indentation suggests you want nested XML elements, but the parent element is an empty tag, so you've actually got a flat namespace here. > > > > 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. What are CLoS IDs and what are they used for ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Expose resource control capabilites on cache bank
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:
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.
Along with vircaps2xmltest case updated.
Signed-off-by: Eli Qiao
---
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,
+ "\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.
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.
+}
+
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
[libvirt] [PATCH] Expose resource control capabilites on cache bank
This patch is based on Martin's cache branch.
This patch amends the cache bank capability as follow:
Along with vircaps2xmltest case updated.
Signed-off-by: Eli Qiao
---
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,
+ "\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;
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;
+}
+
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;
-
/* 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->lev
