On Tuesday, 11 April 2017 at 3:48 PM, Peter Krempa wrote:

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@intel.com>

---

[...]

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);

--

libvir-list mailing list

libvir-list@redhat.com

https://www.redhat.com/mailman/listinfo/libvir-list