Re: [PATCH v8 07/32] x86,fs/resctrl: Refactor domain_remove_cpu_mon() ready for new domain types

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

Hi Tony,

On 8/11/25 11:16 AM, Tony Luck wrote:
> All monitoring events are associated with the L3 resource.
> 
> The RDT_RESOURCE_L3 resource carries a lot of state in the domain
> structures which needs to be dealt with when a domain is taken offline
> by removing the last CPU in the domain.
> 
> New telemetry events will be associated with a new package scoped
> resource with new domain structures.
> 
> Refactor domain_remove_cpu_mon() so all the L3 processing is separated

"separated" -> "separate"

> from general actions of clearing the CPU bit in the mask and removing
> sub-directories from the mon_data directory.
> 
> resctrl_offline_mon_domain() continues to remove domain specific
> directories and files from the "mon_data" directories, but can skip the

"can skip" -> "skips"

> L3 resource specific cleanup when called for other resource types.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 17 +++++++++++------
>  fs/resctrl/rdtgroup.c              |  5 ++++-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 66270c230b4e..448c2bf9837d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -649,17 +649,22 @@ static void domain_remove_cpu_mon(int cpu, struct rdt_resource *r)
>  	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
>  		return;
>  
> -	d = container_of(hdr, struct rdt_mon_domain, hdr);

This is an example where the requirement introduced in patch #5 to use
domain_header_is_valid() before container_of() is broken. The container_of()
is moved but its sanity check does not follow.

I think the domain_header_is_valid() should also move to be right before
the container_of() and when doing so the resource id should be
hardcoded to be RDT_RESOURCE_L3.

> -	hw_dom = resctrl_to_arch_mon_dom(d);
> +	cpumask_clear_cpu(cpu, &hdr->cpu_mask);
> +	if (!cpumask_empty(&hdr->cpu_mask))
> +		return;
>  
> -	cpumask_clear_cpu(cpu, &d->hdr.cpu_mask);
> -	if (cpumask_empty(&d->hdr.cpu_mask)) {
> +	switch (r->rid) {
> +	case RDT_RESOURCE_L3:
> +		d = container_of(hdr, struct rdt_mon_domain, hdr);
> +		hw_dom = resctrl_to_arch_mon_dom(d);
>  		resctrl_offline_mon_domain(r, d);
>  		list_del_rcu(&d->hdr.list);

When making a change please check if same change can be applied to other
parts of the code so that the code remains consistent. Like here where hdr
can be referenced directly that is now done in domain_remove_cpu_ctrl()
and will make these two functions consistent. (oh ...  but that change did
not actually make it into the next patch even though the cover
letter states that it did ...? )

Reinette