3:37 a.m.
-----Original Message-----
From: John Ferlan [mailto:jferlan@redhat.com]
Sent: Wednesday, September 5, 2018 7:59 PM
To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com
Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>;
Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com>
Subject: Re: [libvirt] [PATCH 03/10] conf: Add CMT capability to host
On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> CMT capability for each cache bank, includes -. Maximum CMT monitoring
> groups(sharing with MBM) could be created,
> which reflects the maximum hardware RMID count.
> -. 'cache threshold'.
> -. Statistical information of last level cache, the actual cache
> occupancy.
>
> cache is splitted into 'bank's, each bank MAY have different cache
> configuration, report cache monitoring capability in unit of cache bank.
>
> cache monitor capability is shown as below:
>
> <cache>
> <bank id='0' level='3' type='both'
size='15' unit='MiB' cpus='0-5'>
> <control granularity='768' unit='KiB' type='code'
maxAllocs='8'/>
> <control granularity='768' unit='KiB'
type='data'
> maxAllocs='8'/>
> + <monitor threshold='540672' unit='B'
maxAllocs='176'/>
> + <feature name=llc_occupancy/>
> + </monitor>
> </bank>
> <bank id='1' level='3' type='both'
size='15' unit='MiB' cpus='6-11'>
> <control granularity='768' unit='KiB' type='code'
maxAllocs='8'/>
> <control granularity='768' unit='KiB'
type='data'
> maxAllocs='8'/>
> + <monitor threshold='540672' unit='B'
maxAllocs='176'/>
> + <feature name=llc_occupancy/>
> + </monitor>
>
> </bank>
> </cache>
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com>
> ---
> docs/schemas/capability.rng | 28 ++++++++++++++++++++++++++++
> src/conf/capabilities.c | 17 +++++++++++++++++
> 2 files changed, 45 insertions(+)
>
This output would be combined with part of existing patch2.
Will be combined in next version patch.
> diff --git a/docs/schemas/capability.rng
b/docs/schemas/capability.rng
> index d61515c..67498f1 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -314,6 +314,24 @@
> </attribute>
> </element>
> </zeroOrMore>
> + <zeroOrMore>
> + <element name='monitor'>
> + <attribute name='threshold'>
> + <ref name='unsignedInt'/>
> + </attribute>
> + <attribute name='unit'>
> + <ref name='unit'/>
> + </attribute>
> + <attribute name='maxAllocs'>
> + <ref name='unsignedInt'/>
> + </attribute>
> + <zeroOrMore>
> + <element name='feature'>
> + <ref name='monitorFeature'/>
> + </element>
> + </zeroOrMore>
> + </element>
> + </zeroOrMore>
> </element>
> </oneOrMore>
> </element>
> @@ -329,6 +347,16 @@
> </attribute>
> </define>
>
> + <define name='monitorFeature'>
> + <attribute name='name'>
> + <choice>
> + <value>llc_occupancy</value>
> + <value>mbm_total_bytes</value>
> + <value>mbm_local_bytes</value>
So these are the only 3 values you'll ever expect? Probably not a good idea to
list them like this or the current algorithm is overkill looking for prefixed
"llc_"
and "mbm_" values.
If "llc_somethingnew" shows up some day, then the schema is invalidated.
Disagree.
I don't think the schema will be invalidated when new "llc_somethingnew"
comes. Libvirt only recognize these three feature names.
If a new hardware feature name comes, take your example, the 'llc_somethingnew',
then without a function enabling, should we let it be shown here?
I think properly no. It makes sense to say libvirt only supports the enabled
hardware feature, not all hardware features. To get the new 'llc_somethingnew'
supported here, you need to make changes here and submit the patch.
If all you're supporting or care about is the 3 values, then each
should be fetched
separately. Just the names make me wonder if they come with some associated
value that would be in some file. Perhaps answered in later patches.
Only cares about the 3 values. Will apply more strict name rule to them in source
code in next version patch.
" Just the names make me wonder if they come with some associated
value that would be in some file. Perhaps answered in later patches." -- not
understand.
> + </choice>
> + </attribute>
> + </define>
> +
> <define name='memory_bandwidth'>
> <element name='memory_bandwidth'>
> <oneOrMore>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index
> 5280348..7932088 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -942,6 +942,23 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> controls->max_allocation);
> }
>
> + if (bank->monitor &&
> + bank->monitor->nfeatures) {
> + virBufferAsprintf(&childrenBuf,
> + "<monitor threshold='%u'
unit='B' "
Why is "unit='B' " - does it really matter and is it technically right?
'unit' is the unit of 'threshold'. 'threshold' and 'unit'
reflect the
value reported through resctrl/'max_threshold_occupancy'.
If it's only ever going to be 'B', then easy enough to
document that way.
Realized 'unit' shouldn't be 'B', it could be 'KiB',
'MiB' ....
Should be dynamically changed accordingly. Will be beautified with
'virFormatIntPretty'.
> +
"maxAllocs='%u'>\n",
> + bank->monitor->cache_threshold,
> + bank->monitor->max_allocation);
> + for (j = 0; j < bank->monitor->nfeatures; j++) {
> + virBufferAdjustIndent(&childrenBuf, 2);
> + virBufferAsprintf(&childrenBuf,
> + "<feature
name='%s'/>\n",
> + bank->monitor->features[j]);
> + virBufferAdjustIndent(&childrenBuf, -2);
> + }
> + virBufferAddLit(&childrenBuf, "</monitor>\n");
> + }
> +
Not clear this data is at the right level, still.
I outlined my considerations for putting cache monitor capability under the data structure
of
'cache bank' in my reply of patch 2. It is a bit long, please go to that email
for details.
Again welcome suggestions.
Thanks for review!
Huaqiang
John
> if (virBufferCheckError(&childrenBuf) < 0)
> return -1;
>
>