[libvirt] [PATCH] Capabilities: add step to cache control
Martin Kletzander
mkletzan at redhat.com
Mon Jun 5 13:15:52 UTC 2017
On Mon, Jun 05, 2017 at 06:02:25PM +0800, Eli Qiao wrote:
>This patch adds cache control step size under control tag.
>
>step is calculated by total_cache_size / cbm_length. This will be needed
>while later doing cache allocation to calculate cbm bits.
>
>Besides, count host's cbm length by counting bit instead of checking
>cbm_mask length, along with test case updated.
>
I would rather add another test for that.
>Signed-off-by: Eli Qiao <qiaoliyong at gmail.com>
>---
> docs/schemas/capability.rng | 3 +++
> src/conf/capabilities.c | 19 ++++++++++++++++---
> src/conf/capabilities.h | 1 +
> .../linux-resctrl/resctrl/info/L3/cbm_mask | 2 +-
> .../linux-resctrl/resctrl/info/L3/min_cbm_bits | 2 +-
> .../linux-resctrl/resctrl/info/L3/num_closids | 2 +-
> tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 8 ++++----
> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 4 ++--
> 8 files changed, 29 insertions(+), 12 deletions(-)
>
>diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
>index e5cbfa3..6be7ac9 100644
>--- a/docs/schemas/capability.rng
>+++ b/docs/schemas/capability.rng
>@@ -276,6 +276,9 @@
> <attribute name='min'>
> <ref name='unsignedInt'/>
> </attribute>
>+ <attribute name='step'>
>+ <ref name='unsignedInt'/>
>+ </attribute>
> <attribute name='unit'>
> <ref name='unit'/>
> </attribute>
>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>index 3becc7e..d1266e6 100644
>--- a/src/conf/capabilities.c
>+++ b/src/conf/capabilities.c
>@@ -908,9 +908,10 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> for (j = 0; j < bank->ncontrols; j++) {
> bool min_kilos = !(bank->controls[j]->min % 1024);
> virBufferAsprintf(&controlBuf,
>- "<control min='%llu' unit='%s' "
>+ "<control min='%llu' step='%llu' unit='%s' "
> "type='%s' maxAllocs='%u'/>\n",
> bank->controls[j]->min >> (min_kilos * 10),
>+ bank->controls[j]->step >> (min_kilos * 10),
> min_kilos ? "KiB" : "B",
> virCacheTypeToString(bank->controls[j]->scope),
> bank->controls[j]->max_allocation);
Most of the time the step will be the same as the minimum, at that point
it doesn't make much sense reporting both. Also, you need to check that
both of them are divisible by 2^10, otherwise you might not get exact
results for one of them (e.g. minimum is divisible, but step is not)
>@@ -1602,6 +1603,8 @@ virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank,
> char *cbm_mask = NULL;
> char *type_upper = NULL;
> unsigned int min_cbm_bits = 0;
>+ unsigned int cbm_mask_val = 0;
>+ unsigned int cbm_mask_len = 0;
> virCapsHostCacheControlPtr control;
>
> if (VIR_ALLOC(control) < 0)
>@@ -1632,8 +1635,18 @@ virCapabilitiesGetCacheControl(virCapsHostCacheBankPtr bank,
>
> virStringTrimOptionalNewline(cbm_mask);
>
>- /* cbm_mask: cache bit mask, it's in hex, eg: fffff */
>- control->min = min_cbm_bits * bank->size / (strlen(cbm_mask) * 4);
>+ /* cbm_mask: cache bit mask, it's in hex, e.g. fffff or 7ff */
>+ if (virStrToLong_uip(cbm_mask, NULL, 16, &cbm_mask_val) < 0)
>+ goto cleanup;
>+
The hex numbers get bigger fast. I feel like the unsigned int is not
the right choice for this.
>+ while (cbm_mask_val & 0x1) {
Also counting bit by bit is very tiresome.
>+ cbm_mask_val = cbm_mask_val >> 1;
>+ cbm_mask_len ++;
Extra whitespace.
>+ }
>+
>+ control->step = bank->size / cbm_mask_len;
>+
>+ control->min = min_cbm_bits * control->step;
>
> control->scope = scope;
>
>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
>index ee87d59..2cc9bdc 100644
>--- a/src/conf/capabilities.h
>+++ b/src/conf/capabilities.h
>@@ -152,6 +152,7 @@ typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
> typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
> struct _virCapsHostCacheControl {
> unsigned long long min; /* minimum cache control size in B */
>+ unsigned long long step; /* cache control step size in B */
> virCacheType scope; /* data, code or both */
> unsigned int max_allocation; /* max number of supported allocations */
> };
>diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/cbm_mask b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/cbm_mask
>index 78031da..d482bbb 100644
>--- a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/cbm_mask
>+++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/cbm_mask
>@@ -1 +1 @@
>-fffff
>+7ff
>diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/min_cbm_bits b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/min_cbm_bits
>index 0cfbf08..d00491f 100644
>--- a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/min_cbm_bits
>+++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/min_cbm_bits
>@@ -1 +1 @@
>-2
>+1
The reason for introducing new test was that with this ^^
>diff --git a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/num_closids b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/num_closids
>index b8626c4..b6a7d89 100644
>--- a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/num_closids
>+++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3/num_closids
>@@ -1 +1 @@
>-4
>+16
>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
>index 49aa0b9..98893b6 100644
>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
>@@ -42,12 +42,12 @@
> </topology>
> <cache>
> <bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'>
>- <control min='768' unit='KiB' type='code' maxAllocs='8'/>
>- <control min='768' unit='KiB' type='data' maxAllocs='8'/>
>+ <control min='768' step='768' unit='KiB' type='code' maxAllocs='8'/>
>+ <control min='768' step='768' unit='KiB' type='data' maxAllocs='8'/>
> </bank>
> <bank id='1' level='3' type='both' size='15360' unit='KiB' cpus='6-11'>
>- <control min='768' unit='KiB' type='code' maxAllocs='8'/>
>- <control min='768' unit='KiB' type='data' maxAllocs='8'/>
>+ <control min='768' step='768' unit='KiB' type='code' maxAllocs='8'/>
>+ <control min='768' step='768' unit='KiB' type='data' maxAllocs='8'/>
> </bank>
> </cache>
> </host>
>diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>index cb78b4a..b54f08e 100644
>--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml
>@@ -42,10 +42,10 @@
> </topology>
> <cache>
> <bank id='0' level='3' type='both' size='15360' unit='KiB' cpus='0-5'>
>- <control min='1536' unit='KiB' type='both' maxAllocs='4'/>
>+ <control min='1429876' step='1429876' unit='B' type='both' maxAllocs='16'/>
> </bank>
> <bank id='1' level='3' type='both' size='15360' unit='KiB' cpus='6-11'>
>- <control min='1536' unit='KiB' type='both' maxAllocs='4'/>
>+ <control min='1429876' step='1429876' unit='B' type='both' maxAllocs='16'/>
> </bank>
> </cache>
> </host>
You are not actually checking that those two values are different in any
of the tests.
I cooked up a patch in the meantime as well (although it took me a while
because I had to fix gnulib, too), so there's another idea for the same
thing, let me know what you think about it as well, please:
https://www.redhat.com/archives/libvir-list/2017-June/msg00229.html
Have a nice day,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170605/708d1fce/attachment-0001.sig>
More information about the libvir-list
mailing list