linux-kernel - Re: [PATCH v4 10/31] x86/resctrl: Change generic monitor functions to use struct rdt_domain

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: <ec7c62cc-61a1-42e5-a6b1-066fe7894611@intel.com>
Date: Wed, 7 May 2025 20:38:12 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v4 10/31] x86/resctrl: Change generic monitor functions to
 use struct rdt_domain_hdr

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> Functions that don't need the internal details of the rdt_mon_domain
> can operate on just the rdt_domain_hdr.

Please add a bit more detail in the context:
"just the rdt_domain_hdr" -> "just the rdt_domain_hdr within" or "just rdt_mon_domain::rdt_domain_hdr".

> 
> Add sanity checks where container_of() is used to find the surrounding
> domain structure that hdr has the expected type.

How is reader expected to interpret "hdr"?

> 
> Simplify code that uses "d->hdr." to "hdr->" where possible.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  include/linux/resctrl.h            |  4 +-
>  arch/x86/kernel/cpu/resctrl/core.c | 19 +++----
>  fs/resctrl/rdtgroup.c              | 82 +++++++++++++++++++++---------
>  3 files changed, 68 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index e700f58b5af5..bb55c449adc4 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -444,9 +444,9 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
>  u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
>  			    u32 closid, enum resctrl_conf_type type);
>  int resctrl_online_ctrl_domain(struct rdt_resource *r, struct rdt_ctrl_domain *d);
> -int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d);
> +int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *hdr);
>  void resctrl_offline_ctrl_domain(struct rdt_resource *r, struct rdt_ctrl_domain *d);
> -void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d);
> +void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *hdr);
>  void resctrl_online_cpu(unsigned int cpu);
>  void resctrl_offline_cpu(unsigned int cpu);
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 525439029865..9c78828ae32f 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -460,7 +460,7 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
>  			return;
>  		d = container_of(hdr, struct rdt_ctrl_domain, hdr);

Is the above container_of() still needed?

>  
> -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> +		cpumask_set_cpu(cpu, &hdr->cpu_mask);
>  		if (r->cache.arch_has_per_cpu_cfg)
>  			rdt_domain_reconfigure_cdp(r);
>  		return;
> @@ -524,7 +524,7 @@ static void setup_l3_mon_domain(int cpu, int id, struct rdt_resource *r, struct
>  
>  	list_add_tail_rcu(&d->hdr.list, add_pos);
>  
> -	err = resctrl_online_mon_domain(r, d);
> +	err = resctrl_online_mon_domain(r, &d->hdr);
>  	if (err) {
>  		list_del_rcu(&d->hdr.list);
>  		synchronize_rcu();
> @@ -537,7 +537,6 @@ 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;
> -	struct rdt_mon_domain *d;
>  
>  	lockdep_assert_held(&domain_list_lock);
>  
> @@ -549,11 +548,7 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  
>  	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
>  	if (hdr) {
> -		if (check_domain_header(hdr, RESCTRL_MON_DOMAIN, r->rid))
> -			return;
> -		d = container_of(hdr, struct rdt_mon_domain, hdr);
> -
> -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);

It is not clear to me why changes to domain_add_cpu_ctrl() and domain_add_cpu_mon()
do not match in this regard.

> +		cpumask_set_cpu(cpu, &hdr->cpu_mask);
>  		return;
>  	}
>  
> @@ -603,9 +598,9 @@ static void domain_remove_cpu_ctrl(int cpu, struct rdt_resource *r)
>  	hw_dom = resctrl_to_arch_ctrl_dom(d);
>  
>  	cpumask_clear_cpu(cpu, &d->hdr.cpu_mask);

Above also seems to be candidate for the intended simplification?

> -	if (cpumask_empty(&d->hdr.cpu_mask)) {
> +	if (cpumask_empty(&hdr->cpu_mask)) {
>  		resctrl_offline_ctrl_domain(r, d);
> -		list_del_rcu(&d->hdr.list);
> +		list_del_rcu(&hdr->list);
>  		synchronize_rcu();
>  
>  		/*

Reinette