Messages in this thread | | | Date | Wed, 16 Mar 2022 14:52:04 -0700 | Subject | Re: [PATCH v3 15/21] x86/resctrl: Abstract __rmid_read() | From | Reinette Chatre <> |
| |
Hi James,
On 2/17/2022 10:21 AM, James Morse wrote: > __rmid_read() selects the specified eventid and returns the counter > value from the MSR. The error handling is architecture specific, and > handled by the callers, rdtgroup_mondata_show() and __mon_event_count(). > > Error handling should be handled by architecture specific code, as > a different architecture may have different requirements. MPAM's > counters can report that they are 'not ready', requiring a second > read after a short delay. This should be hidden from resctrl. > > Make __rmid_read() the architecture specific function for reading > a counter. Rename it resctrl_arch_rmid_read() and move the error > handling into it. > > Signed-off-by: James Morse <james.morse@arm.com>
...
> @@ -167,9 +167,9 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d, > memset(am, 0, sizeof(*am)); > } > > -static u64 __rmid_read(u32 rmid, enum resctrl_event_id eventid) > +int resctrl_arch_rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val) > { > - u64 val; > + u64 msr_val; > > /* > * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured > @@ -180,14 +180,24 @@ static u64 __rmid_read(u32 rmid, enum resctrl_event_id eventid) > * are error bits. > */ > wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid); > - rdmsrl(MSR_IA32_QM_CTR, val); > + rdmsrl(MSR_IA32_QM_CTR, msr_val); > > - return val; > + if (msr_val & RMID_VAL_ERROR) > + return -EIO; > + if (msr_val & RMID_VAL_UNAVAIL) > + return -EINVAL; > + > + *val = msr_val; > + > + return 0; > }
From above we see that resctrl_arch_rmid_read() returns an int that could be -EIO or -EINVAL ...
...
> @@ -319,15 +331,15 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr) > { > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(rr->r); > struct mbm_state *m; > - u64 chunks, tval; > + u64 chunks, tval = 0; > > if (rr->first) > resctrl_arch_reset_rmid(rr->r, rr->d, rmid, rr->evtid); > > - tval = __rmid_read(rmid, rr->evtid); > - if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL)) { > - return tval; > - } > + rr->err = resctrl_arch_rmid_read(rmid, rr->evtid, &tval); > + if (rr->err) > + return rr->err; > +
Setting rr->err, an int, to the return of resctrl_arch_rmid_read() is ok and can handle the negative error codes, but returning it here means that __mon_event_count()'s return type should be changed, it is currently u64.
> switch (rr->evtid) { > case QOS_L3_OCCUP_EVENT_ID: > rr->val += tval; > @@ -343,7 +355,7 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr) > * Code would never reach here because an invalid > * event id would fail the __rmid_read. > */ > - return RMID_VAL_ERROR; > + return -EINVAL;
Same here ... __mon_event_count() is u64 still ...
> } > > if (rr->first) { > @@ -419,9 +431,14 @@ void mon_event_count(void *info) > } > } >
Also take care here ... ret_val in mon_event_count() is still u64 while __mon_event_count() attempts to return negative errors.
> - /* Report error if none of rmid_reads are successful */ > - if (ret_val) > - rr->val = ret_val; > + /* > + * __mon_event_count() calls for newly created monitor groups may > + * report -EINVAL/Unavailable if the monitor hasn't seen any traffic. > + * If the first call for the control group succeed, discard any error > + * set by reads of monitor groups. > + */
Additionally, if the first call fails, but a following read of monitor group succeeds then the first call's error is discarded.
How about if the last sentence is replaced with: "Discard error if any of the monitor event reads succeeded."
> + if (ret_val == 0) > + rr->err = 0; > } > > /* > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index 0b48239f5920..70112dbfa128 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -219,6 +219,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_domain *d, > u32 closid, enum resctrl_conf_type type); > int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d); > void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d); > +int resctrl_arch_rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *res); > > /** > * resctrl_arch_reset_rmid() - Reset any private state associated with rmid
Reinette
| |