linux-kernel - Re: [PATCH v3 07/26] x86/resctrl: Move L3 initialization out of domain_add_cpu

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: <aAakETammzi4vGMn@agluck-desk3>
Date: Mon, 21 Apr 2025 13:01:21 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: 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>,
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH v3 07/26] x86/resctrl: Move L3 initialization out of
 domain_add_cpu_mon()

On Fri, Apr 18, 2025 at 02:51:01PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 4/7/25 4:40 PM, Tony Luck wrote:
> > To prepare for additional types of monitoring domains, move all the
> > L3 specific initialization into a helper function.
> > 
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/core.c | 57 ++++++++++++++++++------------
> >  1 file changed, 35 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index d82a4a2db699..703423b0be0e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -493,33 +493,12 @@ 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 setup_l3_mon_domain(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;
> >  	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 (WARN_ON_ONCE(hdr->type != DOMTYPE(r->rid, DOMTYPE_MON)))
> > -			return;
> > -		d = container_of(hdr, struct rdt_mon_domain, hdr);
> > -
> > -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> > -		return;
> > -	}
> > -
> 
> Many functions called by this new "setup_l3_mon_domain()" are specific to L3 resource but
> since L3 resource has so far been the only one supported the naming has been generic. Now that
> this function is made resource specific I think it will help make the code clear if the
> L3 specific functions called by it are also renamed. For example, mon_domain_free() can
> be renamed to free_l3_mon_domain() to match the "setup_l3_mon_domain()" introduced here. Also
> arch_mon_domain_online() -> arch_l3_mon_domain_online().

What about "struct rdt_mon_domain"? It is now specific to L3. Should I
change that to rdt_l3_mon_domain" as well (60 lines affected)?

Ditto for rdt_hw_mon_domain (but only 12 lines for this one).

> Seems like resctrl_online_mon_domain() is only temporarily specific to L3 in this series
> (would be helpful if such details are explained in changelog) ... to start it could
> include a check that ensures it is only called on L3 resource and then it will help
> to clarify what this patch does and how following work builds on it.
> 
> >  	hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
> >  	if (!hw_dom)
> >  		return;
> > @@ -552,6 +531,40 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> >  	}
> >  }
> >  
> Reinette
>