| 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: <050c64b3-20b3-4db6-b782-f5124ebaab31@intel.com>
Date: Fri, 31 May 2024 09:01:04 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>
CC: Fenghua Yu <fenghua.yu@...el.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>,
<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v19 08/20] x86/resctrl: Prepare for new Sub-NUMA Cluster
(SNC) monitor files
Hi Tony,
On 5/30/24 5:26 PM, Tony Luck wrote:
> On Thu, May 30, 2024 at 01:21:01PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 5/28/24 3:19 PM, Tony Luck wrote:
>>> When SNC is enabled monitoring data is collected at the SNC node
>>> granularity, but must be reported at L3-cache granularity for
>>> backwards compatibility in addition to reporting at the node
>>> level.
>>>
>>> Add a "ci" field to the rdt_mon_domain structure to save the
>>> cache information about the enclosing L3 cache for the domain.
>>> This provides:
>>>
>>> 1) The cache id which is needed to compose the name of the legacy
>>> monitoring directory, and to determine which domains should be
>>> summed to provide L3-scoped data.
>>>
>>> 2) The shared_cpu_map which is needed to determine which CPUs can
>>> be used to read the RMID counters with the MSR interface.
>>>
>>> This is the first step to an eventual goal of monitor reporting files
>>> like this (for a system with two SNC nodes per L3):
>>>
>>> $ cd /sys/fs/resctrl/mon_data
>>> $ tree mon_L3_00
>>> mon_L3_00 <- 00 here is L3 cache id
>>> ├── llc_occupancy \ These files provide legacy support
>>> ├── mbm_local_bytes > for non-SNC aware monitor apps
>>> ├── mbm_total_bytes / that expect data at L3 cache level
>>> ├── mon_sub_L3_00 <- 00 here is SNC node id
>>> │ ├── llc_occupancy \ These files are finer grained
>>> │ ├── mbm_local_bytes > data from each SNC node
>>> │ └── mbm_total_bytes /
>>> └── mon_sub_L3_01
>>> ├── llc_occupancy \
>>> ├── mbm_local_bytes > As above, but for node 1.
>>> └── mbm_total_bytes /
>>>
>>> Signed-off-by: Tony Luck <tony.luck@...el.com>
>>> ---
>>> include/linux/resctrl.h | 2 ++
>>> arch/x86/kernel/cpu/resctrl/internal.h | 21 +++++++++++++++++++++
>>> arch/x86/kernel/cpu/resctrl/core.c | 7 ++++++-
>>> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 1 -
>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 1 -
>>> 5 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 64b6ad1b22a1..d733e1f6485d 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -96,6 +96,7 @@ struct rdt_ctrl_domain {
>>> /**
>>> * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
>>> * @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
>>> @@ -106,6 +107,7 @@ struct rdt_ctrl_domain {
>>> */
>>> 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;
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 135190e0711c..eb70d3136ced 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -2,6 +2,7 @@
>>> #ifndef _ASM_X86_RESCTRL_INTERNAL_H
>>> #define _ASM_X86_RESCTRL_INTERNAL_H
>>> +#include <linux/cacheinfo.h>
>>> #include <linux/resctrl.h>
>>> #include <linux/sched.h>
>>> #include <linux/kernfs.h>
>>> @@ -509,6 +510,26 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>>> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>>> +/*
>>> + * Get the cacheinfo structure of the cache associated with @cpu at level @level.
>>> + * cpuhp lock must be held.
>>> + */
>>> +static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
>>> +{
>>> + struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
>>> + int i;
>>> +
>>> + for (i = 0; i < ci->num_leaves; i++) {
>>> + if (ci->info_list[i].level == level) {
>>> + if (ci->info_list[i].attributes & CACHE_ID)
>>> + return &ci->info_list[i];
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>
>> This does not belong in resctrl. It really looks to partner well with existing
>> cache helpers in include/linux/cacheinfo.h that already contains get_cpu_cacheinfo_id().
>> Considering the existing naming get_cpu_cacheinfo() may be more appropriate.
>
> Reinette,
>
> The name get_cpu_cacheinfo() already exists and does something different
> (returns a "struct cpu_cacheinfo *" rather than a "struct cacheinfo *").
Indeed, it is even used by get_cpu_cacheinfo_id() as well as a few
other places in resctrl.
>
> How does this look for the change to <linux/cacheinfo.h> ... add a new
> function get_cpu_cacheinfo_level() and then use it as a helper for the
> existing get_cpu_cacheinfo_id()
>
> -Tony
>
> ---
>
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 2cb15fe4fe12..301b0b24f446 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -113,10 +113,10 @@ int acpi_get_cache_info(unsigned int cpu,
> const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>
> /*
> - * Get the id of the cache associated with @cpu at level @level.
> + * Get the cacheinfo structure for cache associated with @cpu at level @level.
> * cpuhp lock must be held.
> */
> -static inline int get_cpu_cacheinfo_id(int cpu, int level)
> +static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
> {
> struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
> int i;
> @@ -124,12 +124,23 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
> for (i = 0; i < ci->num_leaves; i++) {
> if (ci->info_list[i].level == level) {
> if (ci->info_list[i].attributes & CACHE_ID)
> - return ci->info_list[i].id;
> - return -1;
> + return &ci->info_list[i];
> + return NULL;
> }
> }
>
> - return -1;
> + return NULL;
> +}
> +
> +/*
> + * Get the id of the cache associated with @cpu at level @level.
> + * cpuhp lock must be held.
> + */
> +static inline int get_cpu_cacheinfo_id(int cpu, int level)
> +{
> + struct cacheinfo *ci = get_cpu_cacheinfo_level(cpu, level);
> +
> + return ci ? ci->id : -1;
> }
>
> #ifdef CONFIG_ARM64
This looks useful to me from resctrl perspective. Having it can already
benefit resctrl by replacing the open coded ones in pseudo_lock_region_init()
and rdtgroup_cbm_to_size().
We have previously [1] been able to include a cacheinfo change in a resctrl
series by clearly identifying it as such.
Reinette
[1] https://lore.kernel.org/all/20200708163929.2783-1-james.morse@arm.com/
Powered by blists - more mailing lists