Hi Reinette, On 7/30/25 14:59, Reinette Chatre wrote: > Hi Babu, > > On 7/25/25 11:29 AM, Babu Moger wrote: >> When supported, "mbm_event" counter assignment mode allows users to assign >> a hardware counter to an RMID, event pair and monitor the bandwidth usage >> as long as it is assigned. The hardware continues to track the assigned >> counter until it is explicitly unassigned by the user. >> >> Introduce the architecture calls resctrl_arch_cntr_read() and >> resctrl_arch_reset_cntr() to read and reset event counters when "mbm_event" >> mode is supported. Function names are chosen to match existing > > (apologies if I gave you the text ... trying to polish with more focus on > imperative tone now) > "Function names are chosen to match" -> "Function names match"? Looks good. > >> resctrl_arch_rmid_read() and resctrl_arch_reset_rmid(). >> >> Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- > > ... > >> --- >> include/linux/resctrl.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >> index 50e38445183a..4d37827121a6 100644 >> --- a/include/linux/resctrl.h >> +++ b/include/linux/resctrl.h >> @@ -613,6 +613,44 @@ void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, >> enum resctrl_event_id evtid, u32 rmid, u32 closid, >> u32 cntr_id, bool assign); >> >> +/** >> + * resctrl_arch_cntr_read() - Read the event data corresponding to the counter ID >> + * assigned to the RMID, event pair for this resource >> + * and domain. >> + * @r: Resource that the counter should be read from. >> + * @d: Domain that the counter should be read from. >> + * @closid: CLOSID that matches the RMID. >> + * @rmid: RMID used for counter ID assignment. > > Can this be more specific, for example: > The RMID to which @cntr_id is assigned. Sure. > >> + * @cntr_id: The counter ID whose event data should be read. Valid when >> + * "mbm_event" mode is enabled and @eventid is MBM event. > > Would the counter ID not always be valid? Specifically, resctrl_arch_cntr_read() is > _only_ called when "mbm_event" mode is enabled and @eventid is _always_ > an MBM event, no? If you agree, the @cntr_id description can be something like below > with the calling context details moved to general function description: > > @cntr_id: The counter to read. Yes. that is fine. > >> + * @eventid: eventid used for counter ID assignment, such as >> + * QOS_L3_MBM_TOTAL_EVENT_ID or QOS_L3_MBM_LOCAL_EVENT_ID. > > The "@eventid is an MBM event" can move here? For example: > The MBM event to which @cntr_id is assigned. Sure. > >> + * @val: Result of the counter read in bytes. >> + * > > It looks to me as though some of the @cntr_id text could move to be the > function description. The description can also be expanded to include where this > will be called from. For example, > > Called on a CPU that belongs to domain @d when "mbm_event" mode is enabled. > Called from a non-migrateable process context via smp_call_on_cpu() unless > all CPUs are nohz_full, in which case it is called via IPI (smp_call_function_any()). > > The goal is to make information specific. Please feel free to improve. Looks good. > >> + * Return: >> + * 0 on success, or -EIO, -EINVAL etc on error. >> + */ >> +int resctrl_arch_cntr_read(struct rdt_resource *r, struct rdt_mon_domain *d, >> + u32 closid, u32 rmid, int cntr_id, >> + enum resctrl_event_id eventid, u64 *val); >> + >> +/** >> + * resctrl_arch_reset_cntr() - Reset any private state associated with counter ID. >> + * @r: The domain's resource. >> + * @d: The counter ID's domain. >> + * @closid: CLOSID that matches the RMID. >> + * @rmid: RMID used for counter ID assignment. >> + * @cntr_id: The counter ID whose event data should be reset. Valid when >> + * "mbm_event" mode is enabled and @eventid is MBM event. >> + * @eventid: eventid used for counter ID assignment, such as >> + * QOS_L3_MBM_TOTAL_EVENT_ID or QOS_L3_MBM_LOCAL_EVENT_ID. > > Above should similarly be specific. > Sure. >> + * >> + * This can be called from any CPU. >> + */ >> +void resctrl_arch_reset_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, >> + u32 closid, u32 rmid, int cntr_id, >> + enum resctrl_event_id eventid); >> + >> extern unsigned int resctrl_rmid_realloc_threshold; >> extern unsigned int resctrl_rmid_realloc_limit; >> > > Reinette > -- Thanks Babu Moger
