Re: [libvirt] [PATCH 03/10] conf: Add CMT capability to host

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Saturday, September 8, 2018 1:11 AM 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&gt; Subject: Re: [libvirt] [PATCH 03/10] conf: Add CMT capability to host On 09/07/2018 04:37 AM, Wang, Huaqiang wrote: > > >> -----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&gt; >> 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&gt; >>> --- >>> 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. Hmm... maybe not clear enough - see, virt-xml-validate. Using <choice> limits the choices or allowed values to that known list. So if some kernel some day adds llc_somethingnew, but the libvirt rng file for the customer isn't updated to include that, then the XML doesn't validate. Trying to think of something else similar that exists today, but nothing springs to mind.

> > 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'. > Face-palm on my previous response - KiB not MiB <sigh>, it must be Friday. Oh it is!!! yay!

John >>> + "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; >>> >>>