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

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

 



On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> This patch is based on Martin's cache branch.
> 
> * This patch amends the cache bank capability as follow:
> 
> <cache>
>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>   </bank>
>   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>   </bank>
> </cache>
> 
> For CDP enabled on x86 arch, we will have:
> <cache>
>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>     <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
>     <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
>   </bank>
> ...
> 
> * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
> case.
> 
> * Along with vircaps2xmltest case updated.
> 
> P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
> keep it capital upper as I need to use a macro to convert from enum to
> string easily.

We dont need to do that. The attributes should be lower case. The code
can convert it to anything it needs.

> 
> Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx>
> ---

[...]

> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index 2080953..bfed4f8 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -277,6 +277,26 @@
>            <attribute name='cpus'>
>              <ref name='cpuset'/>
>            </attribute>
> +          <zeroOrMore>
> +            <element name='control'>
> +              <attribute name='min'>
> +                <ref name='unsignedInt'/>
> +              </attribute>
> +              <attribute name='unit'>
> +                <ref name='unit'/>
> +              </attribute>
> +              <attribute name='scope'>
> +                <choice>
> +                  <value>BOTH</value>
> +                  <value>CODE</value>
> +                  <value>DATA</value>

Why are the values all caps? We prefer lowercase attributes in the XML.

> +                </choice>
> +              </attribute>
> +              <attribute name='max_allocation'>
> +                <ref name='unsignedInt'/>
> +              </attribute>
> +            </element>
> +          </zeroOrMore>
>          </element>
>        </oneOrMore>
>      </element>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 416dd1a..c6641d1 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,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>                              virCapsHostCacheBankPtr *caches)
>  {
>      size_t i = 0;
> +    size_t j = 0;
> +    int indent = virBufferGetIndent(buf, false);
> +    virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
>  
>      if (!ncaches)
>          return 0;
> @@ -894,13 +898,38 @@ 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);
>  
> +        /* Format control */
> +        virBufferAdjustIndent(&controlBuf, indent + 4);

This looks wrong. You should increase/decrease the indent by some
number. The old value should not be used.

> +        for (j = 0; j < bank->ncontrols; j++) {
> +            bool min_kilos = !(bank->controls[j]->min % 1024);
> +            virBufferAsprintf(&controlBuf,
> +                              "<control min='%llu' unit='%s' "
> +                              "scope='%s' max_allocation='%u'/>\n",
> +                              bank->controls[j]->min >> (min_kilos * 10),
> +                              min_kilos ? "KiB" : "B",
> +                              virResctrlCacheTypeToString(bank->controls[j]->scope),
> +                              bank->controls[j]->max_allocation);
> +        }
> +
> +        /* Put it all together */

You don't need to point out obvious things.

> +        if (virBufferUse(&controlBuf)) {

This logic looks weird and opaque. Can you decide by any other means
than the filling of the buffer?

> +            virBufferAddLit(buf, ">\n");
> +            virBufferAddBuffer(buf, &controlBuf);
> +            virBufferAddLit(buf, "</bank>\n");
> +
> +        } else {
> +            virBufferAddLit(buf, "/>\n");
> +        }
> +
> +
> +        virBufferFreeAndReset(&controlBuf);
>          VIR_FREE(cpus_str);
>      }
>  
> @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
>  void
>  virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
>  {
> +    size_t i;
> +
>      if (!ptr)
>          return;
>  
>      virBitmapFree(ptr->cpus);
> +    for (i = 0; i < ptr->ncontrols; i++)
> +        VIR_FREE(ptr->controls[i]);
> +    VIR_FREE(ptr->controls);
>      VIR_FREE(ptr);
>  }
>  
> +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> +              "BOTH",
> +              "CODE",
> +              "DATA")
> +
> +/* test which TYPE 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);
> +        /* for CDP enabled case, CODE and DATA will show together */
> +        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,
> +                               virResctrlCacheType scope)
> +{
> +    int ret = -1;
> +    char *path = NULL;
> +    char *cbm_mask = NULL;
> +    virCapsHostCacheControlPtr control;
> +
> +    if (VIR_ALLOC(control) < 0)
> +        goto cleanup;
> +
> +    if (virFileReadValueUint(&control->max_allocation,
> +                             SYSFS_RESCTRL_PATH "info/L%u%s/num_closids",
> +                             bank->level,
> +                             scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> +        goto cleanup;
> +
> +    if (virFileReadValueString(&cbm_mask,
> +                               SYSFS_RESCTRL_PATH
> +                               "info/L%u%s/cbm_mask",
> +                               bank->level,
> +                               scope ? virResctrlCacheTypeToString(scope) : "") < 0)
> +        goto cleanup;
> +
> +    virStringTrimOptionalNewline(cbm_mask);
> +
> +    control->min = bank->size / (strlen(cbm_mask) * 4);

What is this calculating? Using length of a string looks weird. Please
add a comment explaining this.

> +
> +    control->scope = scope;
> +
> +    if (VIR_APPEND_ELEMENT(bank->controls,
> +                           bank->ncontrols,
> +                           control) < 0)
> +        goto error;
> +
> +    ret = 0;

cbm_mask is leaked.

> +
> + cleanup:
> +    VIR_FREE(path);
> +    return ret;
> + error:
> +    VIR_FREE(control);
> +    return -1;

Why do you need an error label?

> +}
> +
>  int
>  virCapabilitiesInitCaches(virCapsPtr caps)
>  {
> @@ -1595,7 +1712,21 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>                                         pos, ent->d_name) < 0)
>                  goto cleanup;
>  
> -            if (bank->level < cache_min_level) {
> +            ret = virCapabilitiesGetCacheControlType(bank);

This overwrites ret. The function uses ret for the final return. This
might break it. I'd suggest you use a different variable.

> +
> +            if ((bank->level >= cache_min_level) || (ret >= 0)) {
> +                if (ret == 0) {

Here ret is 0. (success)

> +                    if (virCapabilitiesGetCacheControl(bank,
> +                                                       VIR_RESCTRL_CACHE_TYPE_BOTH) < 0)
> +                        goto cleanup;

And if this fails you return success.

> +                } else if (ret == 1) {
> +                    if ((virCapabilitiesGetCacheControl(bank,
> +                                                        VIR_RESCTRL_CACHE_TYPE_CODE) < 0) ||
> +                            (virCapabilitiesGetCacheControl(bank,
> +                                                            VIR_RESCTRL_CACHE_TYPE_DATA) < 0))
> +                        goto cleanup;
> +                }
> +            } else {
>                  virCapsHostCacheBankFree(bank);
>                  bank = NULL;
>                  continue;
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index e099ccc..5fd3e57 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -138,10 +138,30 @@ struct _virCapsHostSecModel {
>      virCapsHostSecModelLabelPtr labels;
>  };
>  
> +/* the enum value is equal to virCacheType, but we define a new enum
> +   as for resctrl it has different statement */

I don't know what you wanted to say with this comment.

> +typedef enum {
> +    VIR_RESCTRL_CACHE_TYPE_BOTH,
> +    VIR_RESCTRL_CACHE_TYPE_CODE,
> +    VIR_RESCTRL_CACHE_TYPE_DATA,
> +
> +    VIR_RESCTRL_CACHE_TYPE_LAST
> +} virResctrlCacheType;
> +
> +VIR_ENUM_DECL(virResctrlCache);
> +
> +typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> +struct _virCapsHostCacheControl {
> +    unsigned long long min; /* B */

B ?

> +    virResctrlCacheType scope;  /* Data, Code or Both */
> +    unsigned int max_allocation; /* max number of supported allocations */
> +};
> +
>  typedef enum {
> -    VIR_CACHE_TYPE_DATA,
> -    VIR_CACHE_TYPE_INSTRUCTION,
>      VIR_CACHE_TYPE_UNIFIED,
> +    VIR_CACHE_TYPE_INSTRUCTION,
> +    VIR_CACHE_TYPE_DATA,

This is suspicious. Any explanation for moving them around?

>  
>      VIR_CACHE_TYPE_LAST
>  } virCacheType;
> @@ -156,6 +176,8 @@ struct _virCapsHostCacheBank {
>      unsigned long long size; /* B */
>      virCacheType type;  /* Data, Instruction or Unified */
>      virBitmapPtr cpus;  /* All CPUs that share this bank */
> +    size_t ncontrols;
> +    virCapsHostCacheControlPtr *controls;
>  };
>  
>  typedef struct _virCapsHost virCapsHost;

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

"foo"? Some testing code leftover?

> +        goto cleanup;
> +
> +

Too many empty lines.

> +    virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
>      virFileMockAddPrefix("/sys/devices/system", dir);
>      caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
>  

Attachment: signature.asc
Description: PGP 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