Messages in this thread | ![/](/images/icornerl.gif) | | From | Peter Newman <> | Date | Wed, 14 Dec 2022 15:21:39 +0100 | Subject | Re: [PATCH] x86/resctrl: Fix event counts regression in reused RMIDs |
| |
Hi Reinette,
On Thu, Dec 8, 2022 at 7:31 PM Reinette Chatre <reinette.chatre@intel.com> wrote: > > I think this can be cleaned up to make the code more clear. Notice the > duplication of following snippet in __mon_event_count(): > rr->val += tval; > return 0; > > I do not see any need to check the event id before doing the above. That > leaves the bulk of the switch just needed for the rr->first handling that > can be moved to resctrl_arch_reset_rmid(). > > Something like: > > void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d, ... > { > ... > struct arch_mbm_state *am; > struct mbm_state *m; > u64 val = 0; > int ret; > > m = get_mbm_state(d, rmid, eventid); /* get_mbm_state() to be created */
Good call. When prototyping another change, I quickly found the need to create this myself.
> if (m) > memset(m, 0, sizeof(*m));
mbm_state is arch-independent, so I think putting it here would require the MPAM version to copy this and for get_mbm_state() to be exported.
> > am = get_arch_mbm_state(hw_dom, rmid, eventid); > if (am) { > memset(am, 0, sizeof(*am)); > /* Record any initial, non-zero count value. */ > ret = __rmid_read(rmid, eventid, &val); > if (!ret) > am->prev_msr = val; > } > > } > > Having this would be helpful as reference to Babu's usage.
His usage looks a little different.
According to the comment in Babu's patch:
https://lore.kernel.org/lkml/166990903030.17806.5106229901730558377.stgit@bmoger-ubuntu/
+ /* + * When an Event Configuration is changed, the bandwidth counters + * for all RMIDs and Events will be cleared by the hardware. The + * hardware also sets MSR_IA32_QM_CTR.Unavailable (bit 62) for + * every RMID on the next read to any event for every RMID. + * Subsequent reads will have MSR_IA32_QM_CTR.Unavailable (bit 62) + * cleared while it is tracked by the hardware. Clear the + * mbm_local and mbm_total counts for all the RMIDs. + */ + resctrl_arch_reset_rmid_all(r, d);
If all the hardware counters are zeroed as the comment suggests, then leaving am->prev_msr zero seems correct. __rmid_read() would likely return an error anyways. The bug I was addressing was one of reusing an RMID which had not been reset.
> > Also please note that I changed the __rmid_read(). There is no need > to require each __rmid_read() caller to test MSR bits for validity, that > can be contained within __rmid_read(). > > Something like below remains: > > 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; > } > > rr->err = resctrl_arch_rmid_read(rr->r, rr->d, rmid, rr->evtid, &tval); > if (rr->err) > return rr->err; > > rr->val += tval; > return 0; > > } > > What do you think?
Looks much better. This function has been bothering me since the refactor. I'll see how close I can get to this in the next patch.
Thanks! -Peter
| ![\](/images/icornerr.gif) |