| 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: <8c3aa9cd-335e-415d-a9d3-35593fdbe812@intel.com>
Date: Fri, 11 Apr 2025 14:44:18 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....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 18/26] x86/resctrl: Add default MBM event
configurations for mbm_cntr_assign mode
Hi Babu,
On 4/3/25 5:18 PM, Babu Moger wrote:
> By default, each resctrl group supports two MBM events: mbm_total_bytes
> and mbm_local_bytes. To maintain the same level of support, two default
> MBM configurations are added. These configurations will initially be used
> to set up the counters upon mounting, while users will have the option to
> modify them as needed.
This jumps in quite fast by stating that MBM configurations are added but
there is no definition of what an MBM configuration is.
>
> Event configuration values:
> ========================================================
> Bits Mnemonics Description
> ==== ========================================================
> 6 VictimBW Dirty Victims from all types of memory
> 5 RmtSlowFill Reads to slow memory in the non-local NUMA domain
> 4 LclSlowFill Reads to slow memory in the local NUMA domain
> 3 RmtNTWr Non-temporal writes to non-local NUMA domain
> 2 LclNTWr Non-temporal writes to local NUMA domain
> 1 mtFill Reads to memory in the non-local NUMA domain
> 0 LclFill Reads to memory in the local NUMA domain
> ==== ========================================================
What is the purpose of the mnemonics?
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v12: New patch to support event configurations via new counter_configs
> method.
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++++++
> include/linux/resctrl_types.h | 17 +++++++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d84f47db4e43..aba23e2096db 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -57,6 +57,21 @@ static struct kernfs_node *kn_mongrp;
> /* Kernel fs node for "mon_data" directory under root */
> static struct kernfs_node *kn_mondata;
>
> +struct mbm_evt_value mbm_evt_values[NUM_MBM_EVT_VALUES] = {
> + {"local_reads", 0x1},
> + {"remote_reads", 0x2},
> + {"local_non_temporal_writes", 0x4},
> + {"remote_non_temporal_writes", 0x8},
> + {"local_reads_slow_memory", 0x10},
> + {"remote_reads_slow_memory", 0x20},
> + {"dirty_victim_writes_all", 0x40},
> +};
> +
> +struct mbm_assign_config mbm_assign_configs[NUM_MBM_ASSIGN_CONFIGS] = {
> + {"mbm_total_bytes", QOS_L3_MBM_TOTAL_EVENT_ID, 0x7f},
> + {"mbm_local_bytes", QOS_L3_MBM_LOCAL_EVENT_ID, 0x15},
> +};
> +
> /*
> * Used to store the max resource name width to display the schemata names in
> * a tabular format.
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index f26450b3326b..3d98c7bdb459 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
Please read changelog of f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")
for a good explanation of what resctrl_types.h is used for.
> @@ -31,6 +31,9 @@
> /* Max event bits supported */
> #define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
>
> +#define NUM_MBM_EVT_VALUES 7
> +#define NUM_MBM_ASSIGN_CONFIGS 2
Please keep changes to internal header files unless required.
> +
> enum resctrl_res_level {
> RDT_RESOURCE_L3,
> RDT_RESOURCE_L2,
> @@ -51,4 +54,18 @@ enum resctrl_event_id {
> QOS_L3_MBM_LOCAL_EVENT_ID = 0x03,
> };
>
> +struct mbm_evt_value {
> + char evt_name[32];
> + u32 evt_val;
> +};
I cannot see how this belongs in resctrl_types.h.
> +
> +/**
> + * struct mbm_assign_config - Configuration values
Please include a run of scripts/kernel-doc in your patch preparation steps.
The description "Configuration values" is incredibly vague.
> + */
> +struct mbm_assign_config {
> + char name[32];
> + enum resctrl_event_id evtid;
> + u32 val;
> +};
Why is this new struct needed? It looks to me like a duplicate of struct
mon_evt with one member added. There is also already the evt_list as part
of a monitor resource that the array introduced here seems to duplicate.
Could the event configuration be made a member of struct mon_evt instead?
This exposes the need to integrate this better with BMEC support to make
clear how existing "configurable" member should used and/or expanded.
There seems more and more overlap with Tony's RMID work. Did you get a
chance to look at that?
> +
> #endif /* __LINUX_RESCTRL_TYPES_H */
Reinette
Powered by blists - more mailing lists