linux-kernel - Re: [PATCH V2 1/1] x86/resctrl: Remove unappropriate references to cacheinfo in the resctrl subsystem.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b0b3e665-4c4e-46d3-aae0-37853482938a@intel.com>
Date: Thu, 29 May 2025 10:37:33 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Qinyun Tan <qinyuntan@...ux.alibaba.com>, Tony Luck <tony.luck@...el.com>
CC: "H . Peter Anvin" <hpa@...or.com>, <linux-kernel@...r.kernel.org>,
	<x86@...nel.org>
Subject: Re: [PATCH V2 1/1] x86/resctrl: Remove unappropriate references to
 cacheinfo in the resctrl subsystem.

Hi Qinyun Tan,

Thank you very much. I have a few comments about the changelog that
I think will help explain the issue while aiming to have it follow the
"tip" rules documented in Documentation/process/maintainer-tip.rst. 

On 5/28/25 8:16 PM, Qinyun Tan wrote:
> In the resctrl subsystem's Sub-NUMA Cluster (SNC) mode, the rdt_mon_domain
> structure previously relies on the cacheinfo interface to store L3 cache

"previously relies" -> "relies"

> information (e.g., shared_cpu_map) for monitoring. However, this approach
> introduces risks when CPUs go offline:
> 
> The ci field in rdt_mon_domain is initialized using the first online CPU
> of a NUMA node. When this CPU goes offline, its shared_cpu_map is cleared
> to contain only the offline CPU itself. Subsequently, attempting to read
> counters via smp_call_on_cpu(offline_cpu) would fail, but returning zero
> values for "top-level events" without error indication.

Last sentence of above paragraph can be modified slightly to keep it in
imperative tone:
	Subsequently, attempting to read counters via smp_call_on_cpu(offline_cpu)
	fails (and error ignored), returning zero values for "top-level events"
	without any error indication.

> 
> To resolve these issues:

"To resolve these issues:" can be dropped. There is only one issue and the custom
is for the solution to follow the problem description.

> 
> 1. Replace direct cacheinfo references in struct rdt_mon_domain and struct
> rmid_read with the cacheinfo ID (a unique identifier for the L3 cache).
> 
> 2. The hdr.cpu_mask maintained by resctrl constitutes a subset of

"hdr.cpu_mask" -> "rdt_domain_hdr::cpu_mask"

I do not think "rdt_domain_hdr::cpu_mask" should be defined as a subset of
shared_cpu_map though ... especially since the problem description highlights how
shared_cpu_map can contain offline CPUs. How about:

"rdt_domain_hdr::cpu_mask contains the online CPUs associated with that
domain. When reading ..."

> shared_cpu_map. When reading top-level events, we dynamically select a CPU

drop "we" (no impersonating of code)

Considering the context it may help to be specific here:
"select a CPU" -> "select a (known to be online) CPU"

> from hdr.cpu_mask and utilize its corresponding shared_cpu_map for resctrl

"hdr.cpu_mask" -> "rdt_domain_hdr::cpu_mask"

> to determine valid CPUs for reading RMID counter via the MSR interface.

You can highlight the motivation for doing this. For example, "Considering
all CPUs associated with the L3 cache improves the chances of picking a
housekeeping CPU on which the counter reading work can be queued, avoiding an
unnecessary IPI."

Above is quite a mix of changes. Below aims to put it all together while also
adding more modifications as I am seeing the full picture. Please check for accuracy
and feel free to improve.

	In the resctrl subsystem's Sub-NUMA Cluster (SNC) mode, the rdt_mon_domain
	structure representing a NUMA node relies on the cacheinfo interface
	(rdt_mon_domain::ci) to store L3 cache information (e.g., shared_cpu_map)
	for monitoring. The L3 cache information of a SNC NUMA node determines
	which domains are summed for the "top level" L3-scoped events.

	rdt_mon_domain::ci is initialized using the first online CPU
	of a NUMA node. When this CPU goes offline, its shared_cpu_map is cleared
	to contain only the offline CPU itself.	Subsequently, attempting to read
	counters via smp_call_on_cpu(offline_cpu) fails (and error ignored),
	returning zero values for "top-level events" without any error indication.
 
	Replace the cacheinfo references in struct rdt_mon_domain and struct
	rmid_read with the cacheinfo ID (a unique identifier for the L3 cache).
	
	rdt_domain_hdr::cpu_mask contains the online CPUs associated with that
	domain. When reading "top-level events", select a CPU from
	rdt_domain_hdr::cpu_mask and utilize its L3 shared_cpu_map to determine
	valid CPUs for reading RMID counter via the MSR interface.
	Considering all CPUs associated with the L3 cache improves the chances
	of picking a housekeeping CPU on which the counter reading work can be
	queued, avoiding an unnecessary IPI.

> 
> Fixes: 328ea68874642 ("x86/resctrl: Prepare for new Sub-NUMA Cluster (SNC) monitor files")
> Signed-off-by: Qinyun Tan <qinyuntan@...ux.alibaba.com>
> ---

With changelog polished:
| Reviewed-by: Reinette Chatre <reinette.chatre@...el.com>

Reinette