Re: [PATCH v6 31/42] x86/resctrl: Remove the limit on the number of CLOSID

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

On 2/7/25 10:18 AM, James Morse wrote:
> From: Amit Singh Tomar <amitsinght@marvell.com>
> 
> Resctrl allocates and finds free CLOSID values using the bits of a u32.
> This restricts the number of control groups that can be created by
> user-space.
> 
> MPAM has an architectural limit of 2^16 CLOSID values, Intel x86 could
> be extended beyond 32 values. There is at least one MPAM platform which
> supports more than 32 CLOSID values.
> 
> Replace the fixed size bitmap with calls to the bitmap API to allocate
> an array of a sufficient size.
> 
> ffs() returns '1' for bit 0, hence the existing code subtracts 1 from
> the index to get the CLOSID value. find_first_bit() returns the bit
> number which does not need adjusting.
> 
> Signed-off-by: Amit Singh Tomar <amitsinght@marvell.com>
> [ morse: fixed the off-by-one in the allocator and the wrong
>  not-found value. Removed the limit. Rephrase the commit message. ]
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v5:
>  * This patch got pulled into this series.
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 45 +++++++++++++++++---------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 08fec23a38bf..de79da30d500 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -130,8 +130,8 @@ static bool resctrl_is_mbm_event(int e)
>  }
>  
>  /*
> - * Trivial allocator for CLOSIDs. Since h/w only supports a small number,
> - * we can keep a bitmap of free CLOSIDs in a single integer.
> + * Trivial allocator for CLOSIDs. Use BITMAP APIs to manipulate a bitmap
> + * of free CLOSIDs.
>   *
>   * Using a global CLOSID across all resources has some advantages and
>   * some drawbacks:
> @@ -144,7 +144,7 @@ static bool resctrl_is_mbm_event(int e)
>   * - Our choices on how to configure each resource become progressively more
>   *   limited as the number of resources grows.
>   */
> -static unsigned long closid_free_map;
> +static unsigned long *closid_free_map;
>  static int closid_free_map_len;
>  
>  int closids_supported(void)
> @@ -152,20 +152,30 @@ int closids_supported(void)
>  	return closid_free_map_len;
>  }
>  
> -static void closid_init(void)
> +static int closid_init(void)
>  {
>  	struct resctrl_schema *s;
> -	u32 rdt_min_closid = 32;
> +	u32 rdt_min_closid = ~0;
>  
>  	/* Compute rdt_min_closid across all resources */
>  	list_for_each_entry(s, &resctrl_schema_all, list)
>  		rdt_min_closid = min(rdt_min_closid, s->num_closid);
>  
> -	closid_free_map = BIT_MASK(rdt_min_closid) - 1;
> +	closid_free_map = bitmap_alloc(rdt_min_closid, GFP_KERNEL);
> +	if (!closid_free_map)
> +		return -ENOMEM;
> +	bitmap_fill(closid_free_map, rdt_min_closid);
>  
>  	/* RESCTRL_RESERVED_CLOSID is always reserved for the default group */
> -	__clear_bit(RESCTRL_RESERVED_CLOSID, &closid_free_map);
> +	__clear_bit(RESCTRL_RESERVED_CLOSID, closid_free_map);
>  	closid_free_map_len = rdt_min_closid;
> +
> +	return 0;
> +}
> +
> +static void closid_exit(void)
> +{
> +	bitmap_free(closid_free_map);

With closid_free_map being a global, could this also set
closid_free_map to NULL?

>  }
>  
>  static int closid_alloc(void)
> @@ -182,12 +192,11 @@ static int closid_alloc(void)
>  			return cleanest_closid;
>  		closid = cleanest_closid;
>  	} else {
> -		closid = ffs(closid_free_map);
> -		if (closid == 0)
> +		closid = find_first_bit(closid_free_map, closid_free_map_len);
> +		if (closid == closid_free_map_len)
>  			return -ENOSPC;
> -		closid--;
>  	}
> -	__clear_bit(closid, &closid_free_map);
> +	__clear_bit(closid, closid_free_map);
>  
>  	return closid;
>  }
> @@ -196,7 +205,7 @@ void closid_free(int closid)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	__set_bit(closid, &closid_free_map);
> +	__set_bit(closid, closid_free_map);
>  }
>  
>  /**
> @@ -210,7 +219,7 @@ bool closid_allocated(unsigned int closid)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	return !test_bit(closid, &closid_free_map);
> +	return !test_bit(closid, closid_free_map);
>  }
>  
>  /**
> @@ -2754,20 +2763,22 @@ static int rdt_get_tree(struct fs_context *fc)
>  		goto out_ctx;
>  	}
>  
> -	closid_init();
> +	ret = closid_init();
> +	if (ret)
> +		goto out_schemata_free;
>  
>  	if (resctrl_arch_mon_capable())
>  		flags |= RFTYPE_MON;
>  
>  	ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
>  	if (ret)
> -		goto out_schemata_free;
> +		goto out_closid_exit;
>  
>  	kernfs_activate(rdtgroup_default.kn);
>  
>  	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
>  	if (ret < 0)
> -		goto out_schemata_free;
> +		goto out_closid_exit;
>  
>  	if (resctrl_arch_mon_capable()) {
>  		ret = mongroup_create_dir(rdtgroup_default.kn,
> @@ -2818,6 +2829,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  		kernfs_remove(kn_mongrp);
>  out_info:
>  	kernfs_remove(kn_info);
> +out_closid_exit:
> +	closid_exit();
>  out_schemata_free:
>  	schemata_list_destroy();
>  out_ctx:


With closid_init() called from rdt_get_tree() during mount I expected
closid_exit() to be called from rdt_kill_sb() during unmount ?

Reinette