Re: [libvirt] [PATCH 04/10] test: add test case for resctrl monitor

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Saturday, September 8, 2018 1:14 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 04/10] test: add test case for resctrl monitor On 09/07/2018 05:12 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 04/10] test: add test case for resctrl >> monitor >> >> >> >> On 08/27/2018 07:23 AM, Wang Huaqiang wrote: >>> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; >>> --- >>> .../linux-resctrl/resctrl/info/L3_MON/max_threshold_occupancy | 1 + >>> .../vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features | 3 +++ >>> tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids | 1 + >>> tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 6 ++++++ >>> 4 files changed, 11 insertions(+) >>> create mode 100644 >>> tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshol >>> d_ >>> occupancy create mode 100644 >>> tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features >>> create mode 100644 >>> tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids >>> >> >> And this would be combined with part of patch2 and patch3 >> >>> diff --git >>> a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_thresh >>> ol >>> d_occupancy >>> b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_thresh >>> ol >>> d_occupancy >>> new file mode 100644 >>> index 0000000..77f05e2 >>> --- /dev/null >>> +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_th >>> +++ re >>> +++ shold_occupancy >>> @@ -0,0 +1 @@ >>> +270336 >>> diff --git >>> a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_featur >>> es >>> b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_featur >>> es >>> new file mode 100644 >>> index 0000000..0c57b8d >>> --- /dev/null >>> +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_fe >>> +++ at >>> +++ ures >>> @@ -0,0 +1,3 @@ >>> +llc_occupancy >>> +mbm_total_bytes >>> +mbm_local_bytes >> >> Could/should this list values that aren't prefixed by "llc_" and "mbm_" >> to validate your code? > > Will add some 'fake' features to the list, and make more tests. > To be done in next version patch. > >> >> There's only 1 set of data but it's printed twice - that's the reason >> for my comment in patch2 about duplication of the same data that is unnecessary. >> What if there were 10 bank id's, 100? 1000? - lots of waste. Only 2, no big deal. >> > > <cache> > <bank id='0' level='3' type='both' size='55' unit='MiB' cpus='0-21,44-65'> > <control granularity='2816' unit='KiB' type='both' maxAllocs='16'/> > <monitor threshold='270336' unit='B' maxAllocs='176'> > <feature name='llc_occupancy'/> > </monitor> > </bank> > <bank id='1' level='3' type='both' size='55' unit='MiB' cpus='22-43,66-87'> > <control granularity='2816' unit='KiB' type='both' maxAllocs='16'/> > <monitor threshold='270336' unit='B' maxAllocs='176'> > <feature name='llc_occupancy'/> > </monitor> > </bank> > </cache> > > Above is the cache capabilites section, dumped from my system through > 'virsh capabilities' command. > This is a 2-socket E5-2699v4 CPU(22 core with CAT/CMT enabled and CDP > disabled) system, as you said, the cache monitor capability is printed more than once. > > And I need to point out that the following cache 'control' element is > also printed for twice, it met the same situation you mentioned for cache monitor. > "<control granularity='2816' unit='KiB' type='both' maxAllocs='16'/>" > I think we covered this earlier... I'm still not really clear on what <control> per <bank> really provides other than it's a calculated value based on a few different numbers and honestly I had a hard time following that logic all over the place.

John > After you have read my considerations(in the reply to your review of > patch 2) for reason why I used current disign, if you still think it > not wise to make it duplicated, let's find a proper place to keep the > data and eliminate such kind of duplication. > We can make any changes to make our design more reasonable in the > design stage. > > > Thanks for your kind revew! > > BR > Huaqiang > >> John >> >>> diff --git >>> a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids >>> b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rmids >>> new file mode 100644 >>> index 0000000..1057e9a >>> --- /dev/null >>> +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/num_rm >>> +++ id >>> +++ s >>> @@ -0,0 +1 @@ >>> +176 >>> diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml >>> b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml >>> index 9b00cf0..678fdc9 100644 >>> --- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml >>> +++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml >>> @@ -44,9 +44,15 @@ >>> <cache> >>> <bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'> >>> <control granularity='768' min='1536' unit='KiB' type='both' >>> maxAllocs='4'/> >>> + <monitor threshold='270336' 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' min='1536' unit='KiB' type='both' >>> maxAllocs='4'/> >>> + <monitor threshold='270336' unit='B' maxAllocs='176'> >>> + <feature name='llc_occupancy'/> >>> + </monitor> >>> </bank> >>> </cache> >>> <memory_bandwidth> >>>