Messages in this thread | | | Date | Wed, 7 Dec 2022 11:48:10 -0800 | Subject | Re: [PATCH] x86/resctrl: Fix event counts regression in reused RMIDs | From | Reinette Chatre <> |
| |
Hi Peter,
On 12/7/2022 3:29 AM, Peter Newman wrote: > When creating a new monitoring group, the RMID allocated for it may have > been used by a group which was previously removed. In this case, the > hardware counters will have non-zero values which should be deducted > from what is reported in the new group's counts. > > resctrl_arch_reset_rmid() initializes the prev_msr value for counters to > 0, causing the initial count to be charged to the new group. Resurrect > __rmid_read() and use it to initialize prev_msr correctly.
Thank you very much for catching this.
> > Fixes: 1d81d15db39c ("x86/resctrl: Move mbm_overflow_count() into resctrl_arch_rmid_read()") > Signed-off-by: Peter Newman <peternewman@google.com> > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 39 ++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index efe0c30d3a12..404dd9c472c7 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -146,6 +146,24 @@ static inline struct rmid_entry *__rmid_entry(u32 rmid) > return entry; > } > > +static u64 __rmid_read(u32 rmid, enum resctrl_event_id eventid) > +{ > + u64 val; > + > + /* > + * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured > + * with a valid event code for supported resource type and the bits > + * IA32_QM_EVTSEL.RMID (bits 41:32) are configured with valid RMID, > + * IA32_QM_CTR.data (bits 61:0) reports the monitored data. > + * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62) > + * are error bits. > + */ > + wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid); > + rdmsrl(MSR_IA32_QM_CTR, val); > + > + return val; > +} > + > static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_domain *hw_dom, > u32 rmid, > enum resctrl_event_id eventid) > @@ -170,10 +188,17 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d, > { > struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); > struct arch_mbm_state *am; > + uint64_t val; > > am = get_arch_mbm_state(hw_dom, rmid, eventid); > - if (am) > + if (am) { > memset(am, 0, sizeof(*am)); > + > + /* Record any initial, non-zero count value. */ > + val = __rmid_read(rmid, eventid); > + if (!(val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))) > + am->prev_msr = val; > + } > } > > static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width) > @@ -195,17 +220,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) > return -EINVAL; > > - /* > - * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured > - * with a valid event code for supported resource type and the bits > - * IA32_QM_EVTSEL.RMID (bits 41:32) are configured with valid RMID, > - * IA32_QM_CTR.data (bits 61:0) reports the monitored data. > - * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62) > - * are error bits. > - */ > - wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid); > - rdmsrl(MSR_IA32_QM_CTR, msr_val); > - > + msr_val = __rmid_read(rmid, eventid); > if (msr_val & RMID_VAL_ERROR) > return -EIO; > if (msr_val & RMID_VAL_UNAVAIL) > > base-commit: 76dcd734eca23168cb008912c0f69ff408905235 > -- > 2.39.0.rc0.267.gcb52ba06e7-goog >
To get back to the original behavior before the refactoring it also seems that __mon_event_count() needs to return right after calling resctrl_arch_reset_rmid(). The only caller with rr->first set is when the mon directory is created and the returned values are not used, it is just run to get prev_msr set. This also avoids unnecessarily reading the counters twice.
So, how about:
static int __mon_event_count(u32 rmid, struct rmid_read *rr) {
... if (rr->first) { resctrl_arch_reset_rmid(rr->r, rr->d, rmid, rr->evtid); return 0; } ...
}
Also ... there appears to be a leftover related snippet in __mon_event_count() that does not belong anymore and may still cause incorrect behavior:
static int __mon_event_count(u32 rmid, struct rmid_read *rr) { ... if (rr->first) { memset(m, 0, sizeof(struct mbm_state)); return 0; } ... }
Reinette
| |