| 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: <8cb913f4-feee-de7d-60d0-055e234c6501@intel.com>
Date: Tue, 25 Oct 2022 16:45:41 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>,
<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>
CC: <fenghua.yu@...el.com>, <dave.hansen@...ux.intel.com>,
<x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
<akpm@...ux-foundation.org>, <quic_neeraju@...cinc.com>,
<rdunlap@...radead.org>, <damien.lemoal@...nsource.wdc.com>,
<songmuchun@...edance.com>, <peterz@...radead.org>,
<jpoimboe@...nel.org>, <pbonzini@...hat.com>,
<chang.seok.bae@...el.com>, <pawan.kumar.gupta@...ux.intel.com>,
<jmattson@...gle.com>, <daniel.sneddon@...ux.intel.com>,
<sandipan.das@....com>, <tony.luck@...el.com>,
<james.morse@....com>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <bagasdotme@...il.com>,
<eranian@...gle.com>
Subject: Re: [PATCH v7 06/12] x86/resctrl: Introduce data structure to support
monitor configuration
Hi Babu,
On 10/17/2022 3:26 PM, Babu Moger wrote:
> Add a new field in mon_evt to support Bandwidth Monitoring Event
> Configuration(BMEC) and also update the "mon_features" display.
>
> The sysfs file "mon_features" will display the monitor configuration
> if supported.
>
> Before the change.
> $cat /sys/fs/resctrl/info/L3_MON/mon_features
> llc_occupancy
> mbm_total_bytes
> mbm_local_bytes
>
> After the change if BMEC is supported.
> $cat /sys/fs/resctrl/info/L3_MON/mon_features
> llc_occupancy
> mbm_total_bytes
> mbm_total_config
> mbm_local_bytes
> mbm_local_config
This does not seem to be what the code in this patch does.
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 3 ++-
> arch/x86/kernel/cpu/resctrl/internal.h | 4 +++-
> arch/x86/kernel/cpu/resctrl/monitor.c | 7 ++++++-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 5 ++++-
> 4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index d79f494a4e91..46813b1c50c2 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -814,6 +814,7 @@ static __init bool get_rdt_alloc_resources(void)
> static __init bool get_rdt_mon_resources(void)
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + bool mon_configurable = rdt_cpu_has(X86_FEATURE_BMEC);
>
> if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID);
> @@ -825,7 +826,7 @@ static __init bool get_rdt_mon_resources(void)
> if (!rdt_mon_features)
> return false;
>
> - return !rdt_get_mon_l3_config(r);
> + return !rdt_get_mon_l3_config(r, mon_configurable);
> }
This seems to do a portion of configuration in the calling function, pass the
results of to the actual configuration function where the rest of the configuration is
done. Determining "mon_configurable" really looks like it belongs in
rdt_get_mon_l3_config(). Is it availability of rdt_cpu_has() that prevented
that change? Why not make it available internally to all resctrl code?
Patch 7's mbm_config_rftype_init() can also be moved to rdt_get_mon_l3_config()
to match how other related configs (thread_throttle_mode_init()) are done.
>
> static __init void __check_quirks_intel(void)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 16e3c6e03c79..b458f768f30c 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -63,11 +63,13 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
> * struct mon_evt - Entry in the event list of a resource
> * @evtid: event id
> * @name: name of the event
> + * @configurable: true if the event is configurable
> * @list: entry in &rdt_resource->evt_list
> */
> struct mon_evt {
> enum resctrl_event_id evtid;
> char *name;
> + bool configurable;
> struct list_head list;
> };
>
> @@ -522,7 +524,7 @@ int closids_supported(void);
> void closid_free(int closid);
> int alloc_rmid(void);
> void free_rmid(u32 rmid);
> -int rdt_get_mon_l3_config(struct rdt_resource *r);
> +int rdt_get_mon_l3_config(struct rdt_resource *r, bool configurable);
> void mon_event_count(void *info);
> int rdtgroup_mondata_show(struct seq_file *m, void *arg);
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index efe0c30d3a12..4b8adb7f1c5c 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -746,7 +746,7 @@ static void l3_mon_evt_init(struct rdt_resource *r)
> list_add_tail(&mbm_local_event.list, &r->evt_list);
> }
>
> -int rdt_get_mon_l3_config(struct rdt_resource *r)
> +int rdt_get_mon_l3_config(struct rdt_resource *r, bool configurable)
> {
> unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> @@ -783,6 +783,11 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
> if (ret)
> return ret;
>
> + if (configurable) {
> + mbm_total_event.configurable = true;
> + mbm_local_event.configurable = true;
> + }
> +
> l3_mon_evt_init(r);
>
> r->mon_capable = true;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 1271fd1ae2f3..5f0ef1bf4c78 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1001,8 +1001,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
> struct rdt_resource *r = of->kn->parent->priv;
> struct mon_evt *mevt;
>
> - list_for_each_entry(mevt, &r->evt_list, list)
> + list_for_each_entry(mevt, &r->evt_list, list) {
> seq_printf(seq, "%s\n", mevt->name);
> + if (mevt->configurable)
> + seq_printf(seq, "%s_config\n", mevt->name);
> + }
>
> return 0;
> }
>
If mevt->name is "mbm_total_bytes", then would this not
print "mbm_total_bytes_config"? This is different from "mbm_total_config"
in the changelog and does not match the actual files created in later
patches..
Reinette
Powered by blists - more mailing lists