Re: [PATCH v6 3/9] x86/resctrl: Parse ACPI ERDT table and save CACD cpumask for RMDD domains

[Reinette Chatre]

Hi Chenyu,

On 8/25/26 1:06 AM, Chen Yu wrote:
> On Wed, Aug 19, 2026 at 04:01:33PM -0700, Reinette Chatre wrote:
>> On 7/25/26 2:22 AM, Chen Yu wrote:


>>>  static __init bool get_rdt_resources(void)
>>>  {
>>> +	erdt_init();
>>>  	rdt_alloc_capable = get_rdt_alloc_resources();
>>>  	rdt_mon_capable = get_rdt_mon_resources();
>>>  
>>
>> Functions are not expected to leave dangling state when they return failure. get_rdt_resources()
>> returning false is considered a failure and now the caller is left to clean up the dangling
>> state which is not a familiar pattern to use and thus something that can/will trip people.
>>
>> On top of this this implementation pushes the cleanup very far from even the caller making
>> this unfamiliar pattern even harder to recognize.
>>
>> Please let get_rdt_resources() clean up after itself on failure to find any resources. 
>>
> 
> The original intent was to encapsulate the cleanup logic as a helper function (erdt_exit()), 
> since it is needed in multiple places across the code. All other call sites invoke this function
> when necessary. If we place the cleanup call directly inside resctrl_arch_late_init(), it would
> only execute erdt_exit() once within that function. Alternatively, if we let get_rdt_resources()
> handle cleanup via erdt_exit(), then we would need to add explicit erdt_exit() calls in 
> __resctrl_arch_late_init() for each failure path - for example, when cpuhp_setup_state() fails or
> when resctrl_init() fails. Anyway, if code readability is a priority, I'll change the logic
> as suggested.

Yes, code readability is a priority. It is not all just about code readability. Please consider
all the points I mentioned above. I do not see any justification for get_rdt_resources()
leaving dangling state on failure. This is something that will get tripped over in the next
inevitable refactor. Apart from that, adding a single "cleanup" function at one location down in
the call stack is convenient for *this* implementation based on the *current* state of the code but
while doing so it breaks custom, not just of resctrl but of the rest of the kernel also, and because
of that makes this code difficult to build on and maintain.

> 
>>> @@ -1114,7 +1115,7 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
>>>  	}
>>>  }
>>>  
>>> -static int __init resctrl_arch_late_init(void)
>>> +static int __init __resctrl_arch_late_init(void)
>>>  {
>>>  	struct rdt_resource *r;
>>>  	int state, ret, i;
>>> @@ -1157,6 +1158,15 @@ static int __init resctrl_arch_late_init(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static int __init resctrl_arch_late_init(void)
>>> +{
>>> +	int ret = __resctrl_arch_late_init();
>>> +
>>> +	if (ret)
>>> +		erdt_exit();
>>> +	return ret;
>>> +}
>>
>> Related to earlier comment on cleanup I find this cleanup to be asymmentrical
>> and inconsistent with how resctrl usually does cleanup. Why not do cleanup in
>> (original) resctrl_arch_late_init() to be consistent with other cleanup when
>> failures are encountered during initialization, for example, cpuhp_remove_state()?
>> I find that having the cleanup handled where error is encountered is easier to understand.
>>
> 
> Thomas previously suggested this approach to avoid using goto.
> https://lore.kernel.org/lkml/871pem5jnh.ffs@fw13/
> However, I agree that we can adopt the individual cleanup strategy instead, as it
> will improve code readability.

I see that that Thomas's comment is made in response to this snippet:
> @@ -1130,20 +1131,24 @@ static int __init resctrl_arch_late_init(void)
>  
>  	check_quirks();
>  
> -	if (!get_rdt_resources())
> -		return -ENODEV;
> +	if (!get_rdt_resources()) {
> +		ret = -ENODEV;
> +		goto out;

Above is the issue I mentioned earlier where get_rdt_resources() leaves dangling state.
If it cleaned up after itself on failure as is the custom in the kernel then
resctrl_arch_late_init() does not have to.

If the goal is to avoid goto in __resctrl_arch_late_init() then the failure paths that
follow get_rdt_resources() can just call erdt_exit() directly to make obvious where what
needs to be cleaned up.

As the suggestion is implemented the cleanup is inconsistent with __resctrl_arch_late_init()
doing its own cleanup for code run _after_ erdt_init() and leaving the erdt_init() cleanup
to be done further down the stack by __resctrl_arch_late_init()'s caller. While technically
correct this inconsistency adds unnecessary complexity and another place that will get tripped
over in the next inevitable refactor.

Reinette