Messages in this thread | | | Date | Thu, 8 Sep 2022 18:00:38 +0100 | Subject | Re: [PATCH v6 12/21] x86/resctrl: Calculate bandwidth from the previous __mon_event_count() chunks | From | James Morse <> |
| |
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 ?
>> __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'.
Thanks,
James
| |