| 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: <a470eae9-4368-42df-96d5-0d63c81980af@intel.com>
Date: Wed, 7 May 2025 20:30:28 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>,
Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
<patches@...ts.linux.dev>
Subject: Re: [PATCH v4 02/31] x86,fs/resctrl: Prepare for more monitor events
Hi Tony,
On 4/28/25 5:33 PM, Tony Luck wrote:
> There's a rule in computer programming that objects appear zero,
> once, or many times. So code accordingly.
>
> There are two MBM events and resctrl is coded with a lot of
>
> if (local)
> do one thing
> if (total)
> do a different thing
>
> Change the rdt_ctrl_domain and rdt_hw_mon_domain structures to hold
rdt_ctrl_domain -> rdt_mon_domain
> arrays of pointers to per event data instead of explicit fields for
> total and local bandwidth.
>
> Simplify the code by coding for many events using loops on
"Simplify the code by coding" seems redundant, maybe just "Simplify"?
> which are enabled.
>
> Move resctrl_is_mbm_event() to <linux/resctrl.h> so it
> can be used more widely. Also provide a for_each_mbm_event()
> helper macro.
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> include/linux/resctrl.h | 15 +++++---
> include/linux/resctrl_types.h | 3 ++
> arch/x86/kernel/cpu/resctrl/internal.h | 6 ++--
> arch/x86/kernel/cpu/resctrl/core.c | 38 ++++++++++----------
> arch/x86/kernel/cpu/resctrl/monitor.c | 33 ++++++++++--------
> fs/resctrl/monitor.c | 13 ++++---
> fs/resctrl/rdtgroup.c | 48 ++++++++++++--------------
> 7 files changed, 84 insertions(+), 72 deletions(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 3c5d111aae65..cef9b0ed984c 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -161,8 +161,7 @@ struct rdt_ctrl_domain {
> * @hdr: common header for different domain types
> * @ci: cache info for this domain
> * @rmid_busy_llc: bitmap of which limbo RMIDs are above threshold
> - * @mbm_total: saved state for MBM total bandwidth
> - * @mbm_local: saved state for MBM local bandwidth
> + * @mbm_states: saved state for each QOS MBM event
> * @mbm_over: worker to periodically read MBM h/w counters
> * @cqm_limbo: worker to periodically read CQM h/w counters
> * @mbm_work_cpu: worker CPU for MBM h/w counters
> @@ -172,8 +171,7 @@ struct rdt_mon_domain {
> struct rdt_domain_hdr hdr;
> struct cacheinfo *ci;
> unsigned long *rmid_busy_llc;
> - struct mbm_state *mbm_total;
> - struct mbm_state *mbm_local;
> + struct mbm_state *mbm_states[QOS_NUM_MBM_EVENTS];
> struct delayed_work mbm_over;
> struct delayed_work cqm_limbo;
> int mbm_work_cpu;
> @@ -376,6 +374,15 @@ void resctrl_enable_mon_event(enum resctrl_event_id evtid);
> bool resctrl_is_mon_event_enabled(enum resctrl_event_id evt);
> bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>
> +static inline bool resctrl_is_mbm_event(int e)
This looks like a good time to change the parameter type to enum resctrl_event_id.
> +{
> + return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
> + e <= QOS_L3_MBM_LOCAL_EVENT_ID);
> +}
> +
> +#define for_each_mbm_event(evt) \
> + for (evt = QOS_L3_MBM_TOTAL_EVENT_ID; evt <= QOS_L3_MBM_LOCAL_EVENT_ID; evt++)
> +
> /**
> * resctrl_arch_mon_event_config_write() - Write the config for an event.
> * @config_info: struct resctrl_mon_config_info describing the resource, domain
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a25fb9c4070d..5ef14a24008c 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -47,4 +47,7 @@ enum resctrl_event_id {
> QOS_NUM_EVENTS,
> };
>
> +#define QOS_NUM_MBM_EVENTS (QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
To prevent possible future confusion/churn while making existing code easier to read,
could this be "QOS_NUM_L3_MBM_EVENTS"?
> +#define MBM_EVENT_IDX(evt) ((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
Naming nit: I think "MBM event idx" is close enough to "MBM event id" to cause confusion.
How about "MBM_STATE_IDX"?
> +
> #endif /* __LINUX_RESCTRL_TYPES_H */
...
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index fda579251dba..bf7fde07846b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -160,18 +160,21 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mon_domain *hw_do
> u32 rmid,
> enum resctrl_event_id eventid)
> {
> + struct arch_mbm_state *state;
> +
> switch (eventid) {
> - case QOS_L3_OCCUP_EVENT_ID:
> - return NULL;
> - case QOS_L3_MBM_TOTAL_EVENT_ID:
> - return &hw_dom->arch_mbm_total[rmid];
> - case QOS_L3_MBM_LOCAL_EVENT_ID:
> - return &hw_dom->arch_mbm_local[rmid];
> default:
> /* Never expect to get here */
> WARN_ON_ONCE(1);
> + fallthrough;
> + case QOS_L3_OCCUP_EVENT_ID:
> return NULL;
> + case QOS_L3_MBM_TOTAL_EVENT_ID:
> + case QOS_L3_MBM_LOCAL_EVENT_ID:
> + state = hw_dom->arch_mbm_states[MBM_EVENT_IDX(eventid)];
Please add a "break" here. Although, I find the resulting get_mbm_state() to accomplish
the same in a much simpler way. Why did get_arch_mbm_state() and get_mbm_state() end up
looking so different?
> }
> +
> + return state ? &state[rmid] : NULL;
> }
>
> void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
...
> @@ -346,15 +346,14 @@ static struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
> u32 rmid, enum resctrl_event_id evtid)
> {
> u32 idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> + struct mbm_state *states;
>
> - switch (evtid) {
> - case QOS_L3_MBM_TOTAL_EVENT_ID:
> - return &d->mbm_total[idx];
> - case QOS_L3_MBM_LOCAL_EVENT_ID:
> - return &d->mbm_local[idx];
> - default:
> + if (!resctrl_is_mbm_event(evtid))
> return NULL;
> - }
> +
> + states = d->mbm_states[MBM_EVENT_IDX(evtid)];
> +
> + return states ? &states[idx] : NULL;
> }
>
get_mbm_state() above for comparison.
Reinette
Powered by blists - more mailing lists