Re: [PATCH v10 07/30] x86/resctrl: Check all domains are offline in resctrl_exit()

[Catalin Marinas <catalin.marinas@arm.com>]

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