| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bb7badd8-dddf-41f3-b509-42cdd4539fbd@intel.com>
Date: Wed, 4 Jun 2025 15:21:42 -0700
From: "Keshavamurthy, Anil S" <anil.s.keshavamurthy@...el.com>
To: Tony Luck <tony.luck@...el.com>, Babu Moger <babu.moger@....com>,
"Reinette Chatre" <reinette.chatre@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
James Morse <james.morse@....com>, Drew Fustini <dfustini@...libre.com>,
"Dave Martin" <Dave.Martin@....com>, Chen Yu <yu.c.chen@...el.com>,
<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v5 1/4 UPDATED] x86,fs/resctrl: Consolidate monitor event
descriptions
On 6/4/2025 2:22 PM, Tony Luck wrote:
> There are currently only three monitor events, all associated with
> the RDT_RESOURCE_L3 resource. Growing support for additional events
> will be easier with some restructuring to have a single point in
> file system code where all attributes of all events are defined.
>
> Place all event descriptions into an array mon_event_all[]. Doing
> this has the beneficial side effect of removing the need for
> rdt_resource::evt_list.
>
> Add resctrl_event_id::QOS_FIRST_EVENT for a lower bound on range
> checks for event ids and as the starting index to scan mon_event_all[].
>
> Drop the code that builds evt_list and change the two places where
> the list is scanned to scan mon_event_all[] instead using a new
> helper macro for_each_mon_event().
>
> Architecture code now informs file system code which events are
> available with resctrl_enable_mon_event().
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> include/linux/resctrl.h | 4 +-
> include/linux/resctrl_types.h | 12 ++++--
> fs/resctrl/internal.h | 13 ++++--
> arch/x86/kernel/cpu/resctrl/core.c | 12 ++++--
> fs/resctrl/monitor.c | 63 +++++++++++++++---------------
> fs/resctrl/rdtgroup.c | 11 +++---
> 6 files changed, 66 insertions(+), 49 deletions(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 9ba771f2ddea..1c87e1ed235f 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -269,7 +269,6 @@ enum resctrl_schema_fmt {
> * @mon_domains: RCU list of all monitor domains for this resource
> * @name: Name to use in "schemata" file.
> * @schema_fmt: Which format string and parser is used for this schema.
> - * @evt_list: List of monitoring events
> * @mbm_cfg_mask: Bandwidth sources that can be tracked when bandwidth
> * monitoring events can be configured.
> * @cdp_capable: Is the CDP feature available on this resource
> @@ -287,7 +286,6 @@ struct rdt_resource {
> struct list_head mon_domains;
> char *name;
> enum resctrl_schema_fmt schema_fmt;
> - struct list_head evt_list;
> unsigned int mbm_cfg_mask;
> bool cdp_capable;
> };
> @@ -372,6 +370,8 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
> u32 resctrl_arch_system_num_rmid_idx(void);
> int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid);
> +
> bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>
> /**
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a25fb9c4070d..2dadbc54e4b3 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -34,11 +34,15 @@
> /* Max event bits supported */
> #define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
>
> -/*
> - * Event IDs, the values match those used to program IA32_QM_EVTSEL before
> - * reading IA32_QM_CTR on RDT systems.
> - */
> +/* Event IDs */
> enum resctrl_event_id {
> + /* Must match value of first event below */
> + QOS_FIRST_EVENT = 0x01,
> +
> + /*
> + * These values match those used to program IA32_QM_EVTSEL before
> + * reading IA32_QM_CTR on RDT systems.
> + */
> QOS_L3_OCCUP_EVENT_ID = 0x01,
> QOS_L3_MBM_TOTAL_EVENT_ID = 0x02,
> QOS_L3_MBM_LOCAL_EVENT_ID = 0x03,
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 9a8cf6f11151..71963255ad36 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -52,19 +52,26 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
> }
>
> /**
> - * struct mon_evt - Entry in the event list of a resource
> + * struct mon_evt - Description of a monitor event
> * @evtid: event id
> + * @rid: index of the resource for this event
> * @name: name of the event
> * @configurable: true if the event is configurable
> - * @list: entry in &rdt_resource->evt_list
> + * @enabled: true if the event is enabled
> */
> struct mon_evt {
> enum resctrl_event_id evtid;
> + enum resctrl_res_level rid;
> char *name;
> bool configurable;
> - struct list_head list;
> + bool enabled;
> };
>
> +extern struct mon_evt mon_event_all[QOS_NUM_EVENTS];
> +
> +#define for_each_mon_event(mevt) for (mevt = &mon_event_all[QOS_FIRST_EVENT]; \
> + mevt < &mon_event_all[QOS_NUM_EVENTS]; mevt++)
> +
> /**
> * struct mon_data - Monitoring details for each event file.
> * @list: Member of the global @mon_data_kn_priv_list list.
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 7109cbfcad4f..7b235b7691aa 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -862,12 +862,18 @@ static __init bool get_rdt_mon_resources(void)
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>
> - if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> + if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> + resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID);
> rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID);
> - if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL))
> + }
> + if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> + resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID);
> rdt_mon_features |= (1 << QOS_L3_MBM_TOTAL_EVENT_ID);
> - if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))
> + }
> + if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
> + resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID);
> rdt_mon_features |= (1 << QOS_L3_MBM_LOCAL_EVENT_ID);
> + }
>
> if (!rdt_mon_features)
> return false;
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index bde2801289d3..90093e54a279 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -842,38 +842,39 @@ static void dom_data_exit(struct rdt_resource *r)
> mutex_unlock(&rdtgroup_mutex);
> }
>
> -static struct mon_evt llc_occupancy_event = {
> - .name = "llc_occupancy",
> - .evtid = QOS_L3_OCCUP_EVENT_ID,
> -};
> -
> -static struct mon_evt mbm_total_event = {
> - .name = "mbm_total_bytes",
> - .evtid = QOS_L3_MBM_TOTAL_EVENT_ID,
> -};
> -
> -static struct mon_evt mbm_local_event = {
> - .name = "mbm_local_bytes",
> - .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
> -};
> -
> /*
> - * Initialize the event list for the resource.
> - *
> - * Note that MBM events are also part of RDT_RESOURCE_L3 resource
> - * because as per the SDM the total and local memory bandwidth
> - * are enumerated as part of L3 monitoring.
> + * All available events. Architecture code marks the ones that
> + * are supported by a system using resctrl_enable_mon_event()
> + * to set .enabled.
> */
> -static void l3_mon_evt_init(struct rdt_resource *r)
> +struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
> + [QOS_L3_OCCUP_EVENT_ID] = {
> + .name = "llc_occupancy",
> + .evtid = QOS_L3_OCCUP_EVENT_ID,
> + .rid = RDT_RESOURCE_L3,
> + },
> + [QOS_L3_MBM_TOTAL_EVENT_ID] = {
> + .name = "mbm_total_bytes",
> + .evtid = QOS_L3_MBM_TOTAL_EVENT_ID,
> + .rid = RDT_RESOURCE_L3,
> + },
> + [QOS_L3_MBM_LOCAL_EVENT_ID] = {
> + .name = "mbm_local_bytes",
> + .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
> + .rid = RDT_RESOURCE_L3,
> + },
> +};
As we start adding many more events to this struct(including region
aware specific events), this this becomes too stressful on the eyes to
read.....can you consider simplifying this with #define something like
below in your next version? NOTE: For the feature that I am adding along
with Chen Yu, we started to define a macro as shown below.
#define MON_EVENT(_id, _name, _res) \
[_id] = { \
.name = _name, \
.evtid = _id, \
.rid = _res, \
}
Above #define can to into include/linux/resctrl_types.h file and the
above code reduces to using MON_EVENT as shown below.
struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
MON_EVENT(QOS_L3_OCCUP_EVENT_ID, "llc_occupancy", RDT_RESOURCE_L3),
MON_EVENT(QOS_L3_MBM_TOTAL_EVENT_ID, "mbm_total_bytes",
RDT_RESOURCE_L3),
MON_EVENT(QOS_L3_MBM_LOCAL_EVENT_ID, "mbm_local_bytes",
RDT_RESOURCE_L3),
};
> +
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid)
> {
> - INIT_LIST_HEAD(&r->evt_list);
> + if (WARN_ON_ONCE(eventid < QOS_FIRST_EVENT || eventid >= QOS_NUM_EVENTS))
> + return;
> + if (mon_event_all[eventid].enabled) {
> + pr_warn("Duplicate enable for event %d\n", eventid);
> + return;
> + }
>
> - if (resctrl_arch_is_llc_occupancy_enabled())
> - list_add_tail(&llc_occupancy_event.list, &r->evt_list);
> - if (resctrl_arch_is_mbm_total_enabled())
> - list_add_tail(&mbm_total_event.list, &r->evt_list);
> - if (resctrl_arch_is_mbm_local_enabled())
> - list_add_tail(&mbm_local_event.list, &r->evt_list);
> + mon_event_all[eventid].enabled = true;
> }
>
> /**
> @@ -900,15 +901,13 @@ int resctrl_mon_resource_init(void)
> if (ret)
> return ret;
>
> - l3_mon_evt_init(r);
> -
> if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_TOTAL_EVENT_ID)) {
> - mbm_total_event.configurable = true;
> + mon_event_all[QOS_L3_MBM_TOTAL_EVENT_ID].configurable = true;
> resctrl_file_fflags_init("mbm_total_bytes_config",
> RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
> }
> if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_LOCAL_EVENT_ID)) {
> - mbm_local_event.configurable = true;
> + mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID].configurable = true;
> resctrl_file_fflags_init("mbm_local_bytes_config",
> RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
> }
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index cc37f58b47dd..e4e4b6af5147 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1150,7 +1150,9 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
> struct rdt_resource *r = rdt_kn_parent_priv(of->kn);
> struct mon_evt *mevt;
>
> - list_for_each_entry(mevt, &r->evt_list, list) {
> + for_each_mon_event(mevt) {
> + if (mevt->rid != r->rid || !mevt->enabled)
> + continue;
> seq_printf(seq, "%s\n", mevt->name);
> if (mevt->configurable)
> seq_printf(seq, "%s_config\n", mevt->name);
> @@ -3055,10 +3057,9 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
> struct mon_evt *mevt;
> int ret, domid;
>
> - if (WARN_ON(list_empty(&r->evt_list)))
> - return -EPERM;
> -
> - list_for_each_entry(mevt, &r->evt_list, list) {
> + for_each_mon_event(mevt) {
> + if (mevt->rid != r->rid || !mevt->enabled)
> + continue;
> domid = do_sum ? d->ci->id : d->hdr.id;
> priv = mon_get_kn_priv(r->rid, domid, mevt, do_sum);
> if (WARN_ON_ONCE(!priv))
Powered by blists - more mailing lists