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] [day] [month] [year] [list]

Message-ID: <c33047c5-8ef2-44f6-bad9-7e618d62476e@linux.alibaba.com>
Date: Fri, 30 May 2025 10:03:24 +0800
From: qinyuntan <qinyuntan@...ux.alibaba.com>
To: Reinette Chatre <reinette.chatre@...el.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 Reinette Chatre,

Thank you very much for reviewing my patch and for correcting the commit 
log content. With your modifications, the commit log now looks much more 
reasonable.

I appreciate your patient guidance. I will resend the third version of 
the patch to you shortly.

Best regards,
Qinyun Tan

On 5/30/25 1:37 AM, Reinette Chatre wrote:
> 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