Hi Catalin,
On 5/9/25 4:21 AM, Catalin Marinas wrote:
> 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.
Not all x86 resources support both control and monitoring. For these resources x86
only initializes the arrays it uses. For example, the L2 resource only supports control
and thus only the ctrl_domains list is initialized. When the above loop runs on a resource
like this it will return "true" because it believes there are monitoring domains
online.
Your conclusion that x86 is safe for now may (reason for use of "may" follows) still
be valid since resctrl_online_domains_exist() is only called by x86 from its
resctrl_arch_exit() that is within the "exit.text" section. Here is where I am not
entirely certain of the impact (hence the earlier use of "may") since from what I can
tell the linker does not actually discard "exit.text" on x86 because it defines
RUNTIME_DISCARD_EXIT.
>
>> 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).
Yes, this can be done with a single loop also.
> 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).
I cannot speak to Boris's plans or requirements related to this work.
Reinette