[libvirt] [PATCH V3] 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: <cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> </cache> Along with vircaps2xmltest case updated. Signed-off-by: Eli Qiao <liyong.qiao@intel.com> --- src/conf/capabilities.c | 120 ++++++++++++++++++++++- src/conf/capabilities.h | 13 ++- tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +- tests/vircaps2xmltest.c | 13 +++ 4 files changed, 148 insertions(+), 6 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 416dd1a..3094e48 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; @@ -894,13 +896,32 @@ 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); + if (bank->ncontrol > 0) { + virBufferAddLit(buf, ">\n"); + 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' nallocations='%u'/>\n", + bank->controls[j]->min >> (min_kilos * 10), + min_kilos ? "KiB" : "B", + virCacheTypeToString(bank->controls[j]->type), + bank->controls[j]->nallocations); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</bank>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + VIR_FREE(cpus_str); } @@ -1513,13 +1534,97 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a, void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr) { + size_t i; + if (!ptr) 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->nallocations, + 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) { @@ -1595,7 +1700,18 @@ virCapabilitiesInitCaches(virCapsPtr caps) pos, ent->d_name) < 0) goto cleanup; - if (bank->level < cache_min_level) { + ret = virCapabilitiesGetCacheControlType(bank); + + if ((bank->level >= cache_min_level) || (ret >= 0)) { + if (ret == 0) { + if (virCapabilitiesGetCacheControl(bank, "") < 0) + goto cleanup; + } else if (ret == 1) { + if ((virCapabilitiesGetCacheControl(bank, "CODE") < 0) || + (virCapabilitiesGetCacheControl(bank, "DATA") < 0)) + goto cleanup; + } + } else { virCapsHostCacheBankFree(bank); bank = NULL; continue; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index e099ccc..1007c30 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 nallocations; /* number of supported allocation */ +}; 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-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml index c30ea87..32a9549 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml @@ -41,8 +41,12 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/> - <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/> + <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> + <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> </cache> </host> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c index f590249..3d1ad43 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,17 @@ test_virCapabilities(const void *opaque) data->resctrl ? "/system" : "") < 0) goto cleanup; + if (data->resctrl) { + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) + goto cleanup; + } else { + /* Mock to bad directory to avoid using /sys/fs/resctrl */ + if (VIR_STRDUP(resctrl_dir, "") < 0) + goto cleanup; + } + + virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir); virFileMockAddPrefix("/sys/devices/system", dir); caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate); @@ -84,6 +96,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
On Thu, Apr 06, 2017 at 08:20:56PM +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' type='unified' nallocations='4'/> </bank>
Why do we need to report 'type' on both bank & control elements. Are they really expected to have different values ? 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/ :|
On Thu, Apr 06, 2017 at 01:25:35PM +0100, Daniel P. Berrange wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +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' type='unified' nallocations='4'/> </bank>
Why do we need to report 'type' on both bank & control elements. Are they really expected to have different values ?
That's one of my questions I had way back in some of the previous discussions. Did not get the answer. I suspect there is a reason to whether CDP is enabled or not and it is not just the type of the cache bank itself. If it is, then CDP makes no sense for us at all, actually for anyone who has access to cache information and that would mean bad design of that thing. And that's not something I would expect from this functionality.
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/ :|
On Thursday, 6 April 2017 at 9:04 PM, Martin Kletzander wrote:
On Thu, Apr 06, 2017 at 01:25:35PM +0100, Daniel P. Berrange wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +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' type='unified' nallocations='4'/> </bank>
Why do we need to report 'type' on both bank & control elements. Are they really expected to have different values ?
There’s a discussion from https://www.redhat.com/archives/libvir-list/2017-March/msg01689.html I think I made a mistake here, it should be ’scope’ instead of ’type’ here. No matter what’s it should be, at least we need to identify if the host enabled CDP for L3 cache or not. If enabled CDP on host, the resctrl looks like this: root@s2600wt:/sys/fs/resctrl# cat schemata L3DATA:0=fffff;1=fffff L3CODE:0=fffff;1=fffff root@s2600wt:/sys/fs/resctrl# cat info/L3 L3CODE/ L3DATA/ root@s2600wt:/sys/fs/resctrl# cat info/L3DATA/num_closids 8 root@s2600wt:/sys/fs/resctrl# cat info/L3CODE/num_closids 8 as you can see that num_closids are divided into 2 part (compared to 16 without CDP), but other info are still same root@s2600wt:/sys/fs/resctrl# cat info/L3CODE/cbm_mask fffff root@s2600wt:/sys/fs/resctrl# cat info/L3CODE/min_cbm_bits 1 root@s2600wt:/sys/fs/resctrl# cat info/L3DATA/cbm_mask fffff root@s2600wt:/sys/fs/resctrl# cat info/L3DATA/min_cbm_bits 1 ------------------------------------------ below output are from disable CDP: root@s2600wt:/sys/fs/resctrl# cat schemata L3:0=fffff;1=fffff root@s2600wt:/sys/fs/resctrl# cat info/L3/ cbm_mask min_cbm_bits num_closids root@s2600wt:/sys/fs/resctrl# cat info/L3/cbm_mask fffff root@s2600wt:/sys/fs/resctrl# cat info/L3/min_cbm_bits 1 root@s2600wt:/sys/fs/resctrl# cat info/L3/num_closids 16
That's one of my questions I had way back in some of the previous discussions. Did not get the answer. I suspect there is a reason to whether CDP is enabled or not and it is not just the type of the cache bank itself. If it is, then CDP makes no sense for us at all, actually for anyone who has access to cache information and that would mean bad design of that thing. And that's not something I would expect from this functionality.
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
On Fri, Apr 07, 2017 at 09:51:02AM +0800, Eli Qiao wrote:
On Thursday, 6 April 2017 at 9:04 PM, Martin Kletzander wrote:
On Thu, Apr 06, 2017 at 01:25:35PM +0100, Daniel P. Berrange wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +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' type='unified' nallocations='4'/> </bank>
Why do we need to report 'type' on both bank & control elements. Are they really expected to have different values ?
There’s a discussion from https://www.redhat.com/archives/libvir-list/2017-March/msg01689.html
I think I made a mistake here, it should be ’scope’ instead of ’type’ here.
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have: <bank type='unified'> <control scope='code'/> <control scope='data'/> </bank> And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well. P.S.: It would be clearly visible if you added the test case ;)
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin Could you please advice which should be changed ? LoL This is the output if I enabled CDP <cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache> 1. change nallocations to allocations/max_allocation? 2. change type to scope ? 3. change `instruction` to `code` (with CDP enabled, it called DATA/CODE which is somewhat different from /sys/fs/system/cpu/cpu*/cache/type, and I am now reuse virCacheType defined by Martin, should I define another Type)?
P.S.: It would be clearly visible if you added the test case ;)
On Fri, Apr 07, 2017 at 05:47:54PM +0800, Eli Qiao wrote:
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin
Could you please advice which should be changed ? LoL
This is the output if I enabled CDP
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache>
1. change nallocations to allocations/max_allocation?
Dan said he's fine with both, I'd probably go for max_allocations
2. change type to scope ?
I don't care, pros for both in the previous mail.
3. change `instruction` to `code` (with CDP enabled, it called DATA/CODE which is somewhat different from /sys/fs/system/cpu/cpu*/cache/type, and I am now reuse virCacheType defined by Martin, should I define another Type)?
Neither here do I really care. If nobody has any other opinion by the end of the day, let's just go with scope='both/data/code', with new virCacheScope (it should map to respective values of virCacheType so we can use that in case we will want to) as that is enum you might want to create anyway, so that you have easier translation from/to kernel structures using Type{To,From}String.
P.S.: It would be clearly visible if you added the test case ;)
On Fri, Apr 07, 2017 at 01:04:57PM +0200, Martin Kletzander wrote:
On Fri, Apr 07, 2017 at 05:47:54PM +0800, Eli Qiao wrote:
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin
Could you please advice which should be changed ? LoL
This is the output if I enabled CDP
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache>
1. change nallocations to allocations/max_allocation?
Dan said he's fine with both, I'd probably go for max_allocations
2. change type to scope ?
I don't care, pros for both in the previous mail.
Both attributes take the same enum values, so it is best to be consistent with the attribute name. 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/ :|
On Friday, 7 April 2017 at 7:06 PM, Daniel P. Berrange wrote:
On Fri, Apr 07, 2017 at 01:04:57PM +0200, Martin Kletzander wrote:
On Fri, Apr 07, 2017 at 05:47:54PM +0800, Eli Qiao wrote:
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin
Could you please advice which should be changed ? LoL
This is the output if I enabled CDP
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache>
1. change nallocations to allocations/max_allocation?
Dan said he's fine with both, I'd probably go for max_allocations
2. change type to scope ?
I don't care, pros for both in the previous mail.
Both attributes take the same enum values, so it is best to be consistent with the attribute name.
Thanks very much for Daniel & Martin Forgive me to ping you again, just make sure I am in correct place : If I read your comments correctly, we can go with: <cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/> </bank> </cache> @Daniel, the enum values are same with `type` unified: 0 instruction: 1 data: 2 but scope should be both(0)/code(1)/data(2), so the attribute name will be `scope`. As we need create another Type{to from}String marco for easy read from /sys/fs/resctr/LxDATA/CODE . Regards, Eli
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/ :|
On Fri, Apr 07, 2017 at 08:07:23PM +0800, Eli Qiao wrote:
On Friday, 7 April 2017 at 7:06 PM, Daniel P. Berrange wrote:
On Fri, Apr 07, 2017 at 01:04:57PM +0200, Martin Kletzander wrote:
On Fri, Apr 07, 2017 at 05:47:54PM +0800, Eli Qiao wrote:
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin
Could you please advice which should be changed ? LoL
This is the output if I enabled CDP
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache>
1. change nallocations to allocations/max_allocation?
Dan said he's fine with both, I'd probably go for max_allocations
2. change type to scope ?
I don't care, pros for both in the previous mail.
Both attributes take the same enum values, so it is best to be consistent with the attribute name.
Thanks very much for Daniel & Martin
Forgive me to ping you again, just make sure I am in correct place :
If I read your comments correctly, we can go with:
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/>
</bank> </cache>
@Daniel,
the enum values are same with `type`
unified: 0 instruction: 1 data: 2
but scope should be both(0)/code(1)/data(2), so the attribute name will be
'both' and 'unified' mean the same thing. 'instruction' and 'code' mean the same thing to. So we should use the same terminology for both attributes. IOW, I suggest we use 'both', 'code', and 'data' everywhere. 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/ :|
</bank> </cache>
@Daniel,
the enum values are same with `type`
unified: 0 instruction: 1 data: 2
but scope should be both(0)/code(1)/data(2), so the attribute name will be
'both' and 'unified' mean the same thing.
'instruction' and 'code' mean the same thing to.
So we should use the same terminology for both attributes. IOW, I suggest we use 'both', 'code', and 'data' everywhere.
Hi Daniel/Martin I update V4 patch of “extend resctrl capabilites” [1] I would like to request for comment, this is something different for your suggestion I added the reason in P.S section of the comment message. Looking for your comments. [1] https://www.redhat.com/archives/libvir-list/2017-April/msg00516.html Thanks. Eli.
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/ :|
On Fri, Apr 07, 2017 at 01:10:38PM +0100, Daniel P. Berrange wrote:
On Fri, Apr 07, 2017 at 08:07:23PM +0800, Eli Qiao wrote:
On Friday, 7 April 2017 at 7:06 PM, Daniel P. Berrange wrote:
On Fri, Apr 07, 2017 at 01:04:57PM +0200, Martin Kletzander wrote:
On Fri, Apr 07, 2017 at 05:47:54PM +0800, Eli Qiao wrote:
The name doesn't really matter that much, 'scope' makes a bit more sense, 'type' is consistent with the cache bank specification, I'm fine with any. The big question here was if it is possible to have:
<bank type='unified'> <control scope='code'/> <control scope='data'/> </bank>
And from what you say, the simple answer is "yes". So we need to have the attribute there in the control element as well.
Dan/Martin
Could you please advice which should be changed ? LoL
This is the output if I enabled CDP
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='instruction' nallocations='8'/> <control min='768' unit='KiB' type='data' nallocations='8'/> </bank> </cache>
1. change nallocations to allocations/max_allocation?
Dan said he's fine with both, I'd probably go for max_allocations
2. change type to scope ?
I don't care, pros for both in the previous mail.
Both attributes take the same enum values, so it is best to be consistent with the attribute name.
Thanks very much for Daniel & Martin
Forgive me to ping you again, just make sure I am in correct place :
If I read your comments correctly, we can go with:
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' scope=‘code' max_allocation ='8'/> <control min='768' unit='KiB' scope=‘data' max_allocation ='8'/>
</bank> </cache>
@Daniel,
the enum values are same with `type`
unified: 0 instruction: 1 data: 2
but scope should be both(0)/code(1)/data(2), so the attribute name will be
'both' and 'unified' mean the same thing.
'instruction' and 'code' mean the same thing to.
So we should use the same terminology for both attributes. IOW, I suggest we use 'both', 'code', and 'data' everywhere.
In that case, please also tell me if I should use 'type' or 'scope' for the cache info. I, personally, have never head anyone referring to a "code cache" or "both cache", but I'm fine with anything as long as we're consistent.
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/ :|
On Thu, Apr 06, 2017 at 08:20:56PM +0800, Eli Qiao wrote:
This patch is based on Martin's cache branch.
This patch amends the cache bank capability as follow:
It helps a lot if you wait for a conclusion on a patch before sending another version as soon as you can change one line.
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> </cache>
I know Dan proposed "nallocations", but it sounds like one word. I would rather use "allocations" or "max_allocs" or something understandable. The reason for it? We have no documentation for our capabilities XML. And nobody is trying to create one as far as I know. So at least the naming should be more intuitive.
Along with vircaps2xmltest case updated.
I'm sensing you haven't ran the tests. Am I right?
Signed-off-by: Eli Qiao <liyong.qiao@intel.com> --- src/conf/capabilities.c | 120 ++++++++++++++++++++++- src/conf/capabilities.h | 13 ++- tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +- tests/vircaps2xmltest.c | 13 +++ 4 files changed, 148 insertions(+), 6 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 416dd1a..3094e48 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; @@ -894,13 +896,32 @@ 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);
+ if (bank->ncontrol > 0) { + virBufferAddLit(buf, ">\n"); + 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' nallocations='%u'/>\n", + bank->controls[j]->min >> (min_kilos * 10), + min_kilos ? "KiB" : "B", + virCacheTypeToString(bank->controls[j]->type), + bank->controls[j]->nallocations); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</bank>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } +
There's virBufferAddBuffer() for easier usage of this. Just grep for childrenBuf and you'll see the usage.
VIR_FREE(cpus_str); }
@@ -1513,13 +1534,97 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a, void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr) { + size_t i; + if (!ptr) 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->nallocations, + 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) { @@ -1595,7 +1700,18 @@ virCapabilitiesInitCaches(virCapsPtr caps) pos, ent->d_name) < 0) goto cleanup;
- if (bank->level < cache_min_level) { + ret = virCapabilitiesGetCacheControlType(bank); + + if ((bank->level >= cache_min_level) || (ret >= 0)) { + if (ret == 0) { + if (virCapabilitiesGetCacheControl(bank, "") < 0) + goto cleanup; + } else if (ret == 1) { + if ((virCapabilitiesGetCacheControl(bank, "CODE") < 0) || + (virCapabilitiesGetCacheControl(bank, "DATA") < 0)) + goto cleanup; + } + } else { virCapsHostCacheBankFree(bank); bank = NULL; continue; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index e099ccc..1007c30 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 nallocations; /* number of supported allocation */
"number of supported allocations"
+}; 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;
ncontrols. as I said, look at the code around, try reasoning about differences if it's already inconsistent.
+ virCapsHostCacheControlPtr *controls; };
typedef struct _virCapsHost virCapsHost; diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml index c30ea87..32a9549 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml @@ -41,8 +41,12 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/> - <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/> + <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> + <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> </cache> </host>
You could add new test to also show how it looks on other machines. For example how does this look like when CDP is enabled. Just copy related directories and files (not all of them, just use common sense) from your machine or some machine you have access to and it differs from what we have in the tests already.
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c index f590249..3d1ad43 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;
in this case "dir" should be removed so the naming is more intuitive.
int ret = -1;
/* @@ -58,6 +59,17 @@ test_virCapabilities(const void *opaque) data->resctrl ? "/system" : "") < 0) goto cleanup;
+ if (data->resctrl) { + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) + goto cleanup; + } else { + /* Mock to bad directory to avoid using /sys/fs/resctrl */ + if (VIR_STRDUP(resctrl_dir, "") < 0) + goto cleanup; + } +
Why so complicated? Why not just doing the virAsprintf unconditionally?
+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir); virFileMockAddPrefix("/sys/devices/system", dir); caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
@@ -84,6 +96,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
On Thu, Apr 06, 2017 at 02:46:10PM +0200, Martin Kletzander wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +0800, Eli Qiao wrote:
This patch is based on Martin's cache branch.
This patch amends the cache bank capability as follow:
It helps a lot if you wait for a conclusion on a patch before sending another version as soon as you can change one line.
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> </cache>
I know Dan proposed "nallocations", but it sounds like one word. I would rather use "allocations" or "max_allocs" or something understandable. The reason for it? We have no documentation for our capabilities XML. And nobody is trying to create one as far as I know. So at least the naming should be more intuitive.
I won't mind either of these alternatives. BTW, we really ought to fix the documentation gap too :-) I'm surprised we have gone so long without documenting this key area of functionality! 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/ :|
On Thursday, 6 April 2017 at 8:46 PM, Martin Kletzander wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +0800, Eli Qiao wrote:
This patch is based on Martin's cache branch.
This patch amends the cache bank capability as follow:
It helps a lot if you wait for a conclusion on a patch before sending another version as soon as you can change one line.
Okay, I will keep my patience next time, this time is just because I was working over time yesterday.
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> </cache>
I know Dan proposed "nallocations", but it sounds like one word. I would rather use "allocations" or "max_allocs" or something understandable. The reason for it? We have no documentation for our capabilities XML. And nobody is trying to create one as far as I know. So at least the naming should be more intuitive.
yep, I will wait for the final decision.
Along with vircaps2xmltest case updated.
I'm sensing you haven't ran the tests. Am I right?
Why ? taget@s2600wt:~/libvirt$ ./tests/vircaps2xmltest TEST: vircaps2xmltest .... 4 OK
Signed-off-by: Eli Qiao <liyong.qiao@intel.com (mailto:liyong.qiao@intel.com)> --- src/conf/capabilities.c | 120 ++++++++++++++++++++++- src/conf/capabilities.h | 13 ++- tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +- tests/vircaps2xmltest.c | 13 +++ 4 files changed, 148 insertions(+), 6 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 416dd1a..3094e48 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; @@ -894,13 +896,32 @@ 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);
+ if (bank->ncontrol > 0) { + virBufferAddLit(buf, ">\n"); + 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' nallocations='%u'/>\n", + bank->controls[j]->min >> (min_kilos * 10), + min_kilos ? "KiB" : "B", + virCacheTypeToString(bank->controls[j]->type), + bank->controls[j]->nallocations); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</bank>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } +
There's virBufferAddBuffer() for easier usage of this. Just grep for childrenBuf and you'll see the usage.
Good to know that , thx.
VIR_FREE(cpus_str); }
@@ -1513,13 +1534,97 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a, void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr) { + size_t i; + if (!ptr) 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->nallocations, + 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) { @@ -1595,7 +1700,18 @@ virCapabilitiesInitCaches(virCapsPtr caps) pos, ent->d_name) < 0) goto cleanup;
- if (bank->level < cache_min_level) { + ret = virCapabilitiesGetCacheControlType(bank); + + if ((bank->level >= cache_min_level) || (ret >= 0)) { + if (ret == 0) { + if (virCapabilitiesGetCacheControl(bank, "") < 0) + goto cleanup; + } else if (ret == 1) { + if ((virCapabilitiesGetCacheControl(bank, "CODE") < 0) || + (virCapabilitiesGetCacheControl(bank, "DATA") < 0)) + goto cleanup; + } + } else { virCapsHostCacheBankFree(bank); bank = NULL; continue; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index e099ccc..1007c30 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 nallocations; /* number of supported allocation */
"number of supported allocations"
+}; 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;
ncontrols. as I said, look at the code around, try reasoning about differences if it's already inconsistent.
+ virCapsHostCacheControlPtr *controls; };
typedef struct _virCapsHost virCapsHost; diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml index c30ea87..32a9549 100644 --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml @@ -41,8 +41,12 @@ </cells> </topology> <cache> - <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'/> - <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'/> + <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> + <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> + <control min='768' unit='KiB' type='unified' nallocations='4'/> + </bank> </cache> </host>
You could add new test to also show how it looks on other machines. For example how does this look like when CDP is enabled. Just copy related directories and files (not all of them, just use common sense) from your machine or some machine you have access to and it differs from what we have in the tests already.
Okay, I can do that.
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c index f590249..3d1ad43 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;
in this case "dir" should be removed so the naming is more intuitive.
int ret = -1;
/* @@ -58,6 +59,17 @@ test_virCapabilities(const void *opaque) data->resctrl ? "/system" : "") < 0) goto cleanup;
+ if (data->resctrl) { + if (virAsprintf(&resctrl_dir, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) + goto cleanup; + } else { + /* Mock to bad directory to avoid using /sys/fs/resctrl */ + if (VIR_STRDUP(resctrl_dir, "") < 0) + goto cleanup; + } +
Why so complicated? Why not just doing the virAsprintf unconditionally?
Okay, that’s good,
+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl_dir); virFileMockAddPrefix("/sys/devices/system", dir); caps = virCapabilitiesNew(data->arch, data->offlineMigrate, data->liveMigrate);
@@ -84,6 +96,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@redhat.com (mailto:libvir-list@redhat.com) https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Apr 07, 2017 at 09:38:39AM +0800, Eli Qiao wrote:
On Thursday, 6 April 2017 at 8:46 PM, Martin Kletzander wrote:
On Thu, Apr 06, 2017 at 08:20:56PM +0800, Eli Qiao wrote:
This patch is based on Martin's cache branch.
This patch amends the cache bank capability as follow:
It helps a lot if you wait for a conclusion on a patch before sending another version as soon as you can change one line.
Okay, I will keep my patience next time, this time is just because I was working over time yesterday.
<cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' type='unified' nallocations='4'/> </bank> </cache>
I know Dan proposed "nallocations", but it sounds like one word. I would rather use "allocations" or "max_allocs" or something understandable. The reason for it? We have no documentation for our capabilities XML. And nobody is trying to create one as far as I know. So at least the naming should be more intuitive.
yep, I will wait for the final decision.
Along with vircaps2xmltest case updated.
I'm sensing you haven't ran the tests. Am I right?
Why ?
taget@s2600wt:~/libvirt$ ./tests/vircaps2xmltest TEST: vircaps2xmltest .... 4 OK
All tests, not just one ;) That's why they're there =) (hint: virschematest)
participants (4)
-
Daniel P. Berrange -
Eli Qiao -
Eli Qiao -
Martin Kletzander