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

[John Ferlan]

On 09/07/2018 05:12 AM, Wang, Huaqiang wrote:
> 
> 
>> -----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 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 at intel.com>
>>> ---
>>>  .../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_threshold_
>>> 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_threshol
>>> d_occupancy
>>> b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_threshol
>>> d_occupancy
>>> new file mode 100644
>>> index 0000000..77f05e2
>>> --- /dev/null
>>> +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/max_thre
>>> +++ shold_occupancy
>>> @@ -0,0 +1 @@
>>> +270336
>>> diff --git
>>> a/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features
>>> b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_features
>>> new file mode 100644
>>> index 0000000..0c57b8d
>>> --- /dev/null
>>> +++ b/tests/vircaps2xmldata/linux-resctrl/resctrl/info/L3_MON/mon_feat
>>> +++ 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_rmid
>>> +++ 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>
>>>