Re: [PATCH] x86/resctrl: Remove unnecessary references to cacheinfo in the resctrl subsystem.

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Qinyun,

Thank you for catching this.

On 5/26/25 12:37 AM, Qinyun Tan wrote:
> In the resctrl subsystem's Sub-NUMA Cluster (SNC) mode, the rdt_mon_domain
> structure previously relied on the cacheinfo interface to store L3 cache

"previously relied" -> "relies"

> information (e.g., shared_cpu_map) for monitoring. However, this approach
> introduces risks when CPUs go offline:
> 
> 1. Inconsistency: The ci field in rdt_mon_domain was initialized using the

"was initialized" -> "is initialized"

> first online CPU of a NUMA node. If this CPU later goes offline, the
> shared_cpu_map (managed by the cache subsystem) is cleared, leading to

"is cleared" sounds like the shared_cpu_map is empty. Looking at
cache_shared_cpu_map_remove() it seems that the worst case is when the
CPU goes offline the shared_cpu_map only contains the offline CPU itself.
If there remains a reference to that shared_cpu_map then it will thus be
to a cpumask with an offline CPU. Are you seeing other scenarios?

> incorrect or undefined behavior when accessed via rdt_mon_domain.

Could you please elaborate what "incorrect and undefined behavior" you
encountered?
It looks like reading the top level event would not be able to read
an event from one (or more?) of the online domains since the shared_cpu_map
will contain an offline CPU causing smp_call_on_cpu() to return with a failure
that is not caught ... the symptom may this be that there are no errors but
data is wrong?

> 
> 2. Lifecycle dependency: The cacheinfo structure's lifecycle is managed
> by the cache subsystem, making it unsafe for resctrl to hold
> long-term references.

This is not obvious to me. Could you please elaborate how resctrl could
have a reference to a removed structure?

> 
> To resolve these issues and align with design principles:
> 
> 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. Use hdr.cpu_mask (already maintained by resctrl) to replace
> shared_cpu_map logic for determining valid CPUs for RMID counter reads
> via the MSR interface.

I think it will help to explain why it is ok now to use hdr.cpu_mask instead
of the shared_cpu_map. In support of this you could mention that the original
solution aimed to read the counters on any CPU associated with the L3 cache
that the sub-numa domain forms part of, but this solution uses the
cpumask of one of the sub-numa domains that is known to be associated with
the L3 cache. This is a reduced set of CPUs from previously intended but
known to be accurate. Alternative may be to build a local cpumask from the
all the sub-numa domains but it is not clear to me how much this will
improve things.

Considering this I do not think the references are "unnecessary" as the
subject claims since the solution does not replace the original cpumask with
an identical one.

> 

Needs a "Fixes:" tag.

> Signed-off-by: Qinyun Tan <qinyuntan@linux.alibaba.com>
> ---

Reinette