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

[Ben Horgan <ben.horgan@arm.com>]

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@intel.com>
> 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@intel.com>
> ---
>  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.
""

>  	int ret;
>  
>  	ret = pseudo_lock_region_alloc(plr);
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 0e93cecf6f30..b2ca6394714a 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -3423,8 +3423,8 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>  static void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>  					   struct rdt_domain_hdr *hdr)
>  {
> -	struct kernfs_node *parent_kn;
>  	struct rdtgroup *prgrp, *crgrp;
> +	struct kernfs_node *parent_kn;
>  	struct list_head *head;
>  
>  	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> @@ -3559,9 +3559,9 @@ static int __init_one_rdt_domain(struct rdt_ctrl_domain *d, struct resctrl_schem
>  	struct resctrl_staged_config *cfg;
>  	struct rdt_resource *r = s->res;
>  	u32 used_b = 0, unused_b = 0;
> +	u32 peer_ctl, ctrl_val;
>  	unsigned long tmp_cbm;
>  	enum rdtgrp_mode mode;
> -	u32 peer_ctl, ctrl_val;
>  	int i;
>  
>  	cfg = &d->staged_config[t];


Thanks,

Ben