Messages in this thread | | | Date | Fri, 16 Dec 2022 17:10:11 -0800 | Subject | Re: [PATCH v2 2/2] x86/resctrl: Avoid redundant counter read in __mon_event_count() | From | Reinette Chatre <> |
| |
Hi Peter,
On 12/14/2022 8:08 AM, Peter Newman wrote: > __mon_event_count() does the per-RMID, per-domain work for > user-initiated event count reads and the initialization of new monitor > groups. > > In the initialization case, after resctrl_arch_reset_rmid() calls > __rmid_read() to record an initial count for a new monitor group, it > immediately calls resctrl_arch_rmid_read(). This re-read of the hardware > counter is unnecessary.
"... and the following computations are ignored by the caller during initialization."
> > Following return from resctrl_arch_reset_rmid(), just clear the > mbm_state and return. > > Also, move the mbm_state lookup into the rr->first case, as it's not > needed for regular event count reads: the QOS_L3_OCCUP_EVENT_ID case was > redundant with the accumulating logic at the end of the function.
Starting the above with "Also" creates impression that two logical changes are merged into a single patch. Since this is not the case you can avoid the impression with something like:
Following return from resctrl_arch_reset_rmid(), just clear the mbm_state and return. This involves moving the mbm_state lookup into the rr->first case, as it's not needed for regular event count reads: the QOS_L3_OCCUP_EVENT_ID case was redundant with the accumulating logic at the end of the function.
> > Signed-off-by: Peter Newman <peternewman@google.com> > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 43 ++++++++++++--------------- > 1 file changed, 19 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index 77538abeb72a..e708df478077 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -366,41 +366,36 @@ void free_rmid(u32 rmid) > list_add_tail(&entry->list, &rmid_free_lru); > } > > +static struct mbm_state *get_mbm_state(struct rdt_domain *d, u32 rmid, > + enum resctrl_event_id evtid) > +{ > + switch (evtid) { > + case QOS_L3_MBM_TOTAL_EVENT_ID: > + return &d->mbm_total[rmid]; > + case QOS_L3_MBM_LOCAL_EVENT_ID: > + return &d->mbm_local[rmid]; > + default: > + return NULL; > + } > +} > + > static int __mon_event_count(u32 rmid, struct rmid_read *rr) > { > struct mbm_state *m; > u64 tval = 0; > > - if (rr->first) > + if (rr->first) { > resctrl_arch_reset_rmid(rr->r, rr->d, rmid, rr->evtid); > + m = get_mbm_state(rr->d, rmid, rr->evtid); > + if (m) > + memset(m, 0, sizeof(struct mbm_state)); > + return 0; > + } > > rr->err = resctrl_arch_rmid_read(rr->r, rr->d, rmid, rr->evtid, &tval); > if (rr->err) > return rr->err; > > - switch (rr->evtid) { > - case QOS_L3_OCCUP_EVENT_ID: > - rr->val += tval; > - return 0; > - case QOS_L3_MBM_TOTAL_EVENT_ID: > - m = &rr->d->mbm_total[rmid]; > - break; > - case QOS_L3_MBM_LOCAL_EVENT_ID: > - m = &rr->d->mbm_local[rmid]; > - break; > - default: > - /* > - * Code would never reach here because an invalid > - * event id would fail in resctrl_arch_rmid_read(). > - */ > - return -EINVAL; > - } > - > - if (rr->first) { > - memset(m, 0, sizeof(struct mbm_state)); > - return 0; > - } > - > rr->val += tval; > > return 0;
Thank you very much.
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Reinette
| |