Re: [PATCH v6 12/21] x86/resctrl: Calculate bandwidth from the previous __mon_event_count() chunks

[Reinette Chatre <reinette.chatre@intel.com>]

On 9/8/2022 10:00 AM, James Morse wrote:
> Hi Hao Xin,
> 
> On 07/09/2022 07:47, haoxin wrote:
>> 在 2022/9/2 下午11:48, James Morse 写道:
>>> mbm_bw_count() is only called by the mbm_handle_overflow() worker once a
>>> second. It reads the hardware register, calculates the bandwidth and
>>> updates m->prev_bw_msr which is used to hold the previous hardware register
>>> value.
>>>
>>> Operating directly on hardware register values makes it difficult to make
>>> this code architecture independent, so that it can be moved to /fs/,
>>> making the mba_sc feature something resctrl supports with no additional
>>> support from the architecture.
>>> Prior to calling mbm_bw_count(), mbm_update() reads from the same hardware
>>> register using __mon_event_count().
>>>
>>> Change mbm_bw_count() to use the current chunks value most recently saved
>>> by __mon_event_count(). This removes an extra call to __rmid_read().
>>> Instead of using m->prev_msr to calculate the number of chunks seen,
>>> use the rr->val that was updated by __mon_event_count(). This removes an
>>> extra call to mbm_overflow_count() and get_corrected_mbm_count().
>>> Calculating bandwidth like this means mbm_bw_count() no longer operates
>>> on hardware register values directly.
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index 3e69386cfe00..2d81b6cd9632 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> 
>>> @@ -516,10 +521,12 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain
>>> *d, int rmid)
>>>        */
>>>       if (is_mbm_total_enabled()) {
>>>           rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID;>> +        rr.val = 0;
> 
>> In mbm_update,  there no use the rr.val, so there no need to initialize ？

This patch introduces using rr.val into mbm_bw_count() that is called
from mbm_update(). This patch thus introduces the requirement that rr.val needs
to be initialized. 

> 
>>>           __mon_event_count(rmid, &rr);
>>>       }
>>>       if (is_mbm_local_enabled()) {
>>>           rr.evtid = QOS_L3_MBM_LOCAL_EVENT_ID;
>>> +        rr.val = 0;
> 
>> ditto.
> 
>>>           __mon_event_count(rmid, &rr);
>>>             /*
> 
> No, but this just leaves that problem for someone else to discover the hard way! I think
> its fair for the compiler to complain that addition on an uninitialised field is a bug.
> 
> I'd prefer to keep this as it is on the principle of 'least surprise'.

Yes, please do keep this. If rr.val is not initialized then the software controller will
use wrong data to compute the delta bandwidth.

Reinette