[libvirt] [PATCH 03/10] conf: Add CMT capability to host
Wang, Huaqiang
huaqiang.wang at intel.com
Fri Sep 7 08:37:56 UTC 2018
> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, September 5, 2018 7:59 PM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at 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 at 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;
> >
> >
More information about the libvir-list
mailing list