| 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: <eac72b71-1441-46ed-a3cc-e2204b7eac07@amd.com>
Date: Mon, 14 Apr 2025 10:56:39 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, tony.luck@...el.com,
peternewman@...gle.com
Cc: corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
paulmck@...nel.org, akpm@...ux-foundation.org, thuth@...hat.com,
rostedt@...dmis.org, ardb@...nel.org, gregkh@...uxfoundation.org,
daniel.sneddon@...ux.intel.com, jpoimboe@...nel.org,
alexandre.chartre@...cle.com, pawan.kumar.gupta@...ux.intel.com,
thomas.lendacky@....com, perry.yuan@....com, seanjc@...gle.com,
kai.huang@...el.com, xiaoyao.li@...el.com, kan.liang@...ux.intel.com,
xin3.li@...el.com, ebiggers@...gle.com, xin@...or.com,
sohil.mehta@...el.com, andrew.cooper3@...rix.com, mario.limonciello@....com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
maciej.wieczor-retman@...el.com, eranian@...gle.com
Subject: Re: [PATCH v12 01/26] x86/resctrl: Introduce mbm_total_cfg and
mbm_local_cfg in struct rdt_hw_mon_domain
Hi Reinette,
Thanks for the quick response to the series.
On 4/11/25 15:49, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured to track specific
>> events. The event configuration is domain specific. Event configurations
>> are not stored in resctrl but instead always read from or written to
>> hardware directly when prompted by user space.
>
> Why is this a problem?
I mean it involves an extra MSR read every time use asks for it.
>
>>
>> Read the event configuration from the hardware during domain
>> initialization and store the configuration value in the rdt_hw_mon_domain
>> structure for later use when the user requests to display it.
>
> Why is this required?
Minor optimization.
>
> This series is about adding support for ABMC while this appears to be
> an optimization for BMEC. Even more, as I see it, this optimization makes
> resctrl support of ABMC and BMEC confusing (more below).
>
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v12: Fixed the conflicts due to recent merge.
>> This patch is for BMEC and there is no dependancy on ABMC feature.
>
> Why still do it?
Ok. Will drop it for now.
>
>> Moved it earlier.
>>
>> v11: Resolved minor conflicts due to code displacement. Actual code didnt
>> change.
>>
>> v10: Conflicts due to code displacement. Actual code didnt change.
>>
>> v9: Added Reviewed-by tag. No other changes.
>>
>> v8: Renamed resctrl_mbm_evt_config_init() to arch_mbm_evt_config_init()
>> Minor commit message update.
>>
>> v7: Fixed initializing INVALID_CONFIG_VALUE to mbm_local_cfg in case of error.
>>
>> v6: Renamed resctrl_arch_mbm_evt_config -> resctrl_mbm_evt_config_init
>> Initialized value to INVALID_CONFIG_VALUE if it is not configurable.
>> Minor commit message update.
>>
>> v5: Exported mon_event_config_index_get.
>> Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.
>>
>> v4: Read the configuration information from the hardware to initialize.
>> Added few commit messages.
>> Fixed the tab spaces.
>>
>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>
>> v2: No changes.
>> ---
>> arch/x86/kernel/cpu/resctrl/core.c | 2 ++
>> arch/x86/kernel/cpu/resctrl/internal.h | 9 +++++++++
>> arch/x86/kernel/cpu/resctrl/monitor.c | 26 ++++++++++++++++++++++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +-
>> 4 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index cf29681d01e0..a28de257168f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -558,6 +558,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>> return;
>> }
>>
>> + arch_mbm_evt_config_init(hw_dom);
>> +
>> list_add_tail_rcu(&d->hdr.list, add_pos);
>>
>> err = resctrl_online_mon_domain(r, d);
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c44c5b496355..9846153aa48f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -32,6 +32,9 @@
>> */
>> #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>>
>> +#define INVALID_CONFIG_VALUE U32_MAX
>> +#define INVALID_CONFIG_INDEX UINT_MAX
>> +
>> /**
>> * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
>> * aren't marked nohz_full
>> @@ -335,6 +338,8 @@ struct rdt_hw_ctrl_domain {
>> * @d_resctrl: Properties exposed to the resctrl file system
>> * @arch_mbm_total: arch private state for MBM total bandwidth
>> * @arch_mbm_local: arch private state for MBM local bandwidth
>> + * @mbm_total_cfg: MBM total bandwidth configuration
>> + * @mbm_local_cfg: MBM local bandwidth configuration
>> *
>> * Members of this structure are accessed via helpers that provide abstraction.
>> */
>> @@ -342,6 +347,8 @@ struct rdt_hw_mon_domain {
>> struct rdt_mon_domain d_resctrl;
>> struct arch_mbm_state *arch_mbm_total;
>> struct arch_mbm_state *arch_mbm_local;
>> + u32 mbm_total_cfg;
>> + u32 mbm_local_cfg;
>> };
>
> This introduces an architecture managed per-domain event configuration while
> the rest of the series introduces a resctrl fs managed global event configuration.
> I see this as the start of a source for confusion about how events are configured since
> there is no further connection between this per-domain event configuration maintained
> by the architecture and the global event configuration maintained by resctrl fs.
>
>>
>> static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
>> @@ -504,6 +511,8 @@ void resctrl_file_fflags_init(const char *config, unsigned long fflags);
>> void rdt_staged_configs_clear(void);
>> bool closid_allocated(unsigned int closid);
>> int resctrl_find_cleanest_closid(void);
>> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>> +unsigned int mon_event_config_index_get(u32 evtid);
>>
>> #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>> int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index a93ed7d2a160..abd337fbd01d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1284,6 +1284,32 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>> return 0;
>> }
>>
>> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom)
>> +{
>> + unsigned int index;
>> + u64 msrval;
>> +
>> + /*
>> + * Read the configuration registers QOS_EVT_CFG_n, where <n> is
>> + * the BMEC event number (EvtID).
>> + */
>> + if (mbm_total_event.configurable) {
>
> Please keep an eye on where things are going in the arch/fs split.
> mbm_total_event is private to resctrl fs and arch code cannot reach into it.
> There is the arch helper resctrl_arch_is_evt_configurable() but I also
> think that this helper needs to be reconsidered in the light of ABMC.
ok
>
> Overall I think this ABMC support needs to consider what already exists
> for BMEC support and ensure that both are supported coherently. For example,
> when a monitor domain has a "MBM local bandwidth configuration" then it should
> be obvious what that means.
ok. Agreed. Lets drop these two patches. Lets address ABMC in this series.
--
Thanks
Babu Moger
Powered by blists - more mailing lists