Messages in this thread Patch in this message | | | From | "Yu, Fenghua" <> | Subject | RE: [PATCH] x86/resctrl: Fix event counts regression in reused RMIDs | Date | Wed, 7 Dec 2022 19:26:19 +0000 |
| |
Hi, Peter,
> 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. > > Fixes: 1d81d15db39c ("x86/resctrl: Move mbm_overflow_count() into > resctrl_arch_rmid_read()")
Are you sure the patch fixes 1d81d15db39c? This commit is just a refactoring patch and doesn't change resctrl_arch_reset_rmid().
Shouldn't the patch fix commit fea62d370d7a?
> 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;
Please move this line to below:
> > am = get_arch_mbm_state(hw_dom, rmid, eventid); > - if (am) > + if (am) {
here: + u64 val;
And it's better to keep the same type name "u64" as other values.
> 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
Thanks.
-Fenghua
| |