linux-kernel - Re: [PATCH v10 07/30] x86/resctrl: Check all domains are offline in resctrl

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: <aB3lS8K7I0wEkGw_@arm.com>
Date: Fri, 9 May 2025 12:21:47 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: James Morse <james.morse@....com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	H Peter Anvin <hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
	shameerali.kolothum.thodi@...wei.com,
	D Scott Phillips OS <scott@...amperecomputing.com>,
	carl@...amperecomputing.com, lcherian@...vell.com,
	bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
	baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
	Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
	dfustini@...libre.com, amitsinght@...vell.com,
	David Hildenbrand <david@...hat.com>,
	Rex Nie <rex.nie@...uarmicro.com>,
	Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
	Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com,
	Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v10 07/30] x86/resctrl: Check all domains are offline in
 resctrl_exit()

Hi Reinette,

Thanks for the reviews.

On Thu, May 08, 2025 at 10:51:23AM -0700, Reinette Chatre wrote:
> On 5/8/25 10:18 AM, James Morse wrote:
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 88197afbbb8a..f617ac97758b 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -4420,8 +4420,32 @@ int __init resctrl_init(void)
> >  	return ret;
> >  }
> >  
> > +static bool __exit resctrl_online_domains_exist(void)
> > +{
> > +	struct rdt_resource *r;
> > +
> > +	for_each_rdt_resource(r) {
> > +		if (!list_empty(&r->ctrl_domains) || !list_empty(&r->mon_domains))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> 
> This looks the same as before. Did you notice my comment in [1] about this list_empty() usage?

James may have missed that comment (and he's off today). Copying your
comment here:

> A list needs to be initialized for list_empty() to behave as intended. A list within
> an uninitialized or "kzalloc()'ed" struct will not be considered empty.
> resctrl_arch_get_resource() as used by for_each_rdt_resource() already establishes
> that if an architecture does not support a particular resource then it can (should?)
> return a "dummy/not-capable" resource. I do not think resctrl should require
> anything additionally like initializing the lists of a dummy/not-capable resource
> to support things like this loop.

I agree the description of the resctrl_arch_get_resource() semantics
doesn't specifically mention that the list_heads should be initialised
in dummy resources. IIUC, x86 should be safe for now since the
rdt_resources_all[] array elements have the ctrl_domains and mon_domains
list_heads initialised.

> Considering this, could this be made more specific? For example,
> 
> 	for_each_alloc_capable_rdt_resource(r) {
> 		if (!list_empty(&r->ctrl_domains))
> 			return true;
> 	}
> 
> 	for_each_mon_capable_rdt_resource(r) {
> 		if (!list_empty(&r->mon_domains))
> 			return true;
> 	}

If not-capable resources can be only partially initialised, your
proposal makes (alternatively, keep a single loop but only check
list_empty() if {mon,alloc}_capable; maybe the compiler is smart enough
to squash the two loops).

That said, if Boris plans to take this series, I don't think it's worth
a respin (assuming my understanding is correct and there are no other
issues with the series). The fix can be added subsequently on top before
other architectures gain support for resctrl (well, most likely Mon/Tue
when James is back).

Thanks.

-- 
Catalin