Re: [PATCH v10 02/28] x86/resctrl: Move L3 initialization into new helper function

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

Hi Tony,

On 9/12/25 3:10 PM, Tony Luck wrote:
> All resctrl monitor events are associated with the L3 resource, but
> this is about to change.

Please see Boris's feedback about changelogs in [1]. To address that,
please rework the changelogs to not have so much copy&pasted context 
in patches. 

> 
> To prepare for additional types of monitoring domains, move open coded L3
> resource monitoring domain initialization from domain_add_cpu_mon() into
> a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 57 +++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 8be2619db2e7..055df4d406d0 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -496,37 +496,13 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
>  	}
>  }
>  
> -static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> +static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
>  {
> -	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> -	struct list_head *add_pos = NULL;
>  	struct rdt_hw_mon_domain *hw_dom;
> -	struct rdt_domain_hdr *hdr;
>  	struct rdt_mon_domain *d;
>  	struct cacheinfo *ci;
>  	int err;
>  
> -	lockdep_assert_held(&domain_list_lock);
> -
> -	if (id < 0) {
> -		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> -			     cpu, r->mon_scope, r->name);
> -		return;
> -	}
> -
> -	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> -	if (hdr) {
> -		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
> -			return;
> -		d = container_of(hdr, struct rdt_mon_domain, hdr);
> -
> -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> -		/* Update the mbm_assign_mode state for the CPU if supported */
> -		if (r->mon.mbm_cntr_assignable)
> -			resctrl_arch_mbm_cntr_assign_set_one(r);

Rebase error? Note the assignable counter initialization done on CPU online ...

> -		return;
> -	}
> -
>  	hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
>  	if (!hw_dom)
>  		return;
> @@ -565,6 +541,37 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  	}
>  }
>  
> +static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> +{
> +	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> +	struct list_head *add_pos = NULL;
> +	struct rdt_domain_hdr *hdr;
> +
> +	lockdep_assert_held(&domain_list_lock);
> +
> +	if (id < 0) {
> +		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> +			     cpu, r->mon_scope, r->name);
> +		return;
> +	}
> +
> +	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> +	if (hdr) {
> +		cpumask_set_cpu(cpu, &hdr->cpu_mask);
> +

... assignable counter initialization no longer done on CPU online.

Looking closer the l3_mon_domain_setup() also now contains assignable counter
initialization that is gated by a RDT_RESOURCE_L3 check. Considering the flow
I think it may thus be simpler and consistent to not return here
but instead have the additional initialization done in resource specific
areas below. 

> +		return;
> +	}
> +
> +	switch (r->rid) {
> +	case RDT_RESOURCE_L3:

Something like:
		if (hdr) {
			/* do resource specific CPU initialization here */
			return;
		}

> +		l3_mon_domain_setup(cpu, id, r, add_pos);
> +		break;
> +	default:
> +		pr_warn_once("Unknown resource rid=%d\n", r->rid);
> +		break;
> +	}
> +}
> +
>  static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  {
>  	if (r->alloc_capable)

Reinette