Hi Tony,
On 6/26/25 9:49 AM, Tony Luck wrote:
> The resctrl file system layer passed the domain, rmid, and event id to
> resctrl_arch_rmid_read() to fetch an event counter.
Please write in present tense and use uppercase for acronyms.
The resctrl file system layer passes the domain, RMID, and event id to
resctrl_arch_rmid_read() to fetch an event counter.
>
> For some resources this may not be enough information to efficiently
> access the counter.
>
> Add mon_evt::arch_priv void pointer. Architecture code can initialize
> this when marking each event enabled.
>
> File system code passes this pointer to resctrl_arch_rmid_read().
>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> include/linux/resctrl.h | 6 ++++--
> fs/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/core.c | 6 +++---
> arch/x86/kernel/cpu/resctrl/monitor.c | 2 +-
> fs/resctrl/monitor.c | 12 ++++++++----
> 5 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 76c54b81e426..b9f2690bee1e 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -383,7 +383,8 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>
> #define MAX_BINARY_BITS 27
>
> -void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu, u32 binary_bits);
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
> + u32 binary_bits, void *arch_priv);
>
> bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid);
>
> @@ -478,6 +479,7 @@ void resctrl_arch_pre_mount(void);
> * only.
> * @rmid: rmid of the counter to read.
> * @eventid: eventid to read, e.g. L3 occupancy.
> + * @arch_priv: architecture private data for this event.
Please append some detail on how it is used. For example,
"Architecture private data for this event. The @arch_priv provided by
the architecture via resctrl_enable_mon_event()."
> * @val: result of the counter read in bytes.
> * @arch_mon_ctx: An architecture specific value from
> * resctrl_arch_mon_ctx_alloc(), for MPAM this identifies
> @@ -495,7 +497,7 @@ void resctrl_arch_pre_mount(void);
> */
> int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain_hdr *hdr,
> u32 closid, u32 rmid, enum resctrl_event_id eventid,
> - u64 *val, void *arch_mon_ctx);
> + void *arch_priv, u64 *val, void *arch_mon_ctx);
>
> /**
> * resctrl_arch_rmid_read_context_check() - warn about invalid contexts
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 53ced959a27d..2126006075f3 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -71,6 +71,7 @@ struct mon_evt {
> bool is_floating_point;
> int binary_bits;
> bool enabled;
> + void *arch_priv;
> };
>
> extern struct mon_evt mon_event_all[QOS_NUM_EVENTS];
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 9144766da836..f3144fe918dd 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -909,15 +909,15 @@ static __init bool get_rdt_mon_resources(void)
> bool ret = false;
>
> if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> - resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0);
> + resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> ret = true;
> }
> if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> - resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false, 0);
> + resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false, 0, NULL);
> ret = true;
> }
> if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
> - resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false, 0);
> + resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false, 0, NULL);
> ret = true;
> }
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 043f777378a6..185b203f6321 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -219,7 +219,7 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
>
> int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain_hdr *hdr,
> u32 unused, u32 rmid, enum resctrl_event_id eventid,
> - u64 *val, void *ignored)
> + void *arch_priv, u64 *val, void *ignored)
> {
> int cpu = cpumask_any(&hdr->cpu_mask);
> struct rdt_hw_l3_mon_domain *hw_dom;
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index cff8af3a263e..c4b092aec9f8 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -160,7 +160,7 @@ void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>
> entry = __rmid_entry(idx);
> if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid, entry->rmid,
> - QOS_L3_OCCUP_EVENT_ID, &val,
> + QOS_L3_OCCUP_EVENT_ID, NULL, &val,
This is resctrl fs code. To maintain clear separation it should not assume architecture
behavior, which this does by setting arch_priv to NULL because x86 does not use it.
> arch_mon_ctx)) {
> rmid_dirty = true;
> } else {
> @@ -402,7 +402,8 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> return -EINVAL;
>
> rr->err = resctrl_arch_rmid_read(rr->r, rr->hdr, closid, rmid,
> - rr->evt->evtid, &tval, rr->arch_mon_ctx);
> + rr->evt->evtid, rr->evt->arch_priv,
> + &tval, rr->arch_mon_ctx);
> if (rr->err)
> return rr->err;
>
> @@ -430,7 +431,8 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> if (d->ci_id != rr->ci_id)
> continue;
> err = resctrl_arch_rmid_read(rr->r, &d->hdr, closid, rmid,
> - rr->evt->evtid, &tval, rr->arch_mon_ctx);
> + rr->evt->evtid, rr->evt->arch_priv,
> + &tval, rr->arch_mon_ctx);
> if (!err) {
> rr->val += tval;
> ret = 0;
> @@ -902,7 +904,8 @@ struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
> MON_EVENT(PMT_EVENT_UOPS_RETIRED, "uops_retired", RDT_RESOURCE_PERF_PKG, false),
> };
>
> -void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu, u32 binary_bits)
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
> + u32 binary_bits, void *arch_priv)
> {
> if (WARN_ON_ONCE(eventid < QOS_FIRST_EVENT || eventid >= QOS_NUM_EVENTS) ||
> binary_bits > MAX_BINARY_BITS)
> @@ -918,6 +921,7 @@ void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu, u32 b
>
> mon_event_all[eventid].any_cpu = any_cpu;
> mon_event_all[eventid].binary_bits = binary_bits;
> + mon_event_all[eventid].arch_priv = arch_priv;
> mon_event_all[eventid].enabled = true;
> }
>
Reinette