Re: [PATCH 00/11] x86,fs/resctrl: Improve resctrl quality and consistency

[Reinette Chatre]

Hi Ben and Tony,

On 3/4/26 3:48 AM, Ben Horgan wrote:
> Hi Tony,
> 
> On 3/2/26 23:37, Luck, Tony wrote:
>> On Mon, Mar 02, 2026 at 10:46:06AM -0800, Reinette Chatre wrote:
>>> Hi Everybody,
>>>
>>> This is a collection of resctrl cleanups assembled together for convenience
>>> and simpler tracking. I'd be happy to split them up if it makes review and/or
>>> handling easier.
>>
>> If it is time for spring cleaning in the rescctrl code, maybe fix some
>> bad fir tree declarations too?
>>
>> Note resctrl_arch_pseudo_lock_fn() needs help too, but complicated by
>> having #ifdef CONFIG_KASAN mixed in with declarations. It might need
>> to remain an exception.
>>
>> -Tony
>>
>>
>> From dd9c2ad1a1361b34e25fc10d18d3ceb3ba57fb92 Mon Sep 17 00:00:00 2001
>> From: Tony Luck <tony.luck@xxxxxxxxx>
>> Date: Mon, 2 Mar 2026 15:28:36 -0800
>> Subject: [PATCH] fs/resctrl: Clean up some bad "fir tree" declarations
>>
>> Sort local variables by length (longest first) per TIP tree.
>>
>> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
>> ---
>>  fs/resctrl/pseudo_lock.c | 2 +-
>>  fs/resctrl/rdtgroup.c    | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/resctrl/pseudo_lock.c b/fs/resctrl/pseudo_lock.c
>> index e1e9134474f4..cd57d862e0cf 100644
>> --- a/fs/resctrl/pseudo_lock.c
>> +++ b/fs/resctrl/pseudo_lock.c
>> @@ -797,10 +797,10 @@ static const struct file_operations pseudo_measure_fops = {
>>  int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
>>  {
>>  	struct pseudo_lock_region *plr = rdtgrp->plr;
>> +	char *kn_name __free(kfree) = NULL;
>>  	struct task_struct *thread;
>>  	unsigned int new_minor;
>>  	struct device *dev;
>> -	char *kn_name __free(kfree) = NULL;
> 
> If you are changing this, I would expect either the declaration to come
> lower at the allocation or alternatively drop the __free and do an
> explict kfree. This is based on the guidance in include/linux/cleanup.h
> 
> ""
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
>  *
>  * Lastly, given that the benefit of cleanup helpers is removal of
>  * "goto", and that the "goto" statement can jump between scopes, the
>  * expectation is that usage of "goto" and cleanup helpers is never
>  * mixed in the same function. I.e. for a given routine, convert all
>  * resources that need a "goto" cleanup to scope-based cleanup, or
>  * convert none of them.
> ""

Since this is the only cleanup helper in this function it is not clear to
me what impact moving of the declaration would have. I fully agree with the latter
though, the usage of the single cleanup helper in this function that continues
to heavily use goto makes the flow through this function difficult to follow.

Reinette