Re: [PATCH v8 10/25] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters

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

Hi Babu,

On 10/9/24 10:39 AM, Babu Moger wrote:
> Hardware provides a set of counters when mbm_assign_mode is supported.
> These counters are assigned to the MBM monitoring events of a MON group
> that needs to be tracked. The kernel must manage and track the available
> counters.
> 
> Introduce mbm_cntr_free_map bitmap to track available counters and set
> of routines to allocate and free the counters. Move dom_data_init() after
> mbm_cntr_assign detection.

Regarding "Move dom_data_init() after mbm_cntr_assign detection." - this is
clear from the patch, please use changelog to explain *why*.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---


> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 ++
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 43 +++++++++++++++++++++++---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 ++++++++++++
>  include/linux/resctrl.h                |  2 ++
>  4 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 92eae4672312..99f9103a35ba 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -654,6 +654,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free);
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  void __init resctrl_file_fflags_init(const char *config,
>  				     unsigned long fflags);
> +int mbm_cntr_alloc(struct rdt_resource *r);
> +void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id);
>  void rdt_staged_configs_clear(void);
>  bool closid_allocated(unsigned int closid);
>  int resctrl_find_cleanest_closid(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 66b06574f660..5c2a28565747 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -983,6 +983,27 @@ void mbm_setup_overflow_handler(struct rdt_mon_domain *dom, unsigned long delay_
>  		schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
>  }
>  
> +/*
> + * Counter bitmap for tracking the available counters.
> + * 'mbm_cntr_assign' mode provides set of hardware counters for assigning
> + * RMID, event pair. Each RMID and event pair takes one hardware counter.
> + */

"counters for assigning RMID, event pair" sounds strange and it seems like the same
thing is mentioned twice.
How about:
	Bitmap tracking the available hardware counters when operating in
	"mbm_cntr_assign" mode. A hardware counter can be assigned to a
	RMID, event pair.

> +static __init unsigned long *mbm_cntrs_init(struct rdt_resource *r)
> +{
> +	r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
> +						 GFP_KERNEL);
> +	if (r->mon.mbm_cntr_free_map)
> +		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
> +
> +	return r->mon.mbm_cntr_free_map;
> +}
> +
> +static  __exit void mbm_cntrs_exit(struct rdt_resource *r)
> +{
> +	bitmap_free(r->mon.mbm_cntr_free_map);
> +	r->mon.mbm_cntr_free_map = NULL;
> +}
> +
>  static __init int dom_data_init(struct rdt_resource *r)
>  {
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> @@ -1020,6 +1041,17 @@ static __init int dom_data_init(struct rdt_resource *r)
>  		goto out_unlock;
>  	}
>  
> +	if (r->mon.mbm_cntr_assignable && !mbm_cntrs_init(r)) {
> +		if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +			kfree(closid_num_dirty_rmid);
> +			closid_num_dirty_rmid = NULL;
> +		}
> +		kfree(rmid_ptrs);
> +		rmid_ptrs = NULL;
> +		err = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
>  	for (i = 0; i < idx_limit; i++) {
>  		entry = &rmid_ptrs[i];
>  		INIT_LIST_HEAD(&entry->list);
> @@ -1056,6 +1088,9 @@ static void __exit dom_data_exit(struct rdt_resource *r)
>  	kfree(rmid_ptrs);
>  	rmid_ptrs = NULL;
>  
> +	if (r->mon.mbm_cntr_assignable)
> +		mbm_cntrs_exit(r);
> +
>  	mutex_unlock(&rdtgroup_mutex);
>  }
>  
> @@ -1210,10 +1245,6 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  	 */
>  	resctrl_rmid_realloc_threshold = resctrl_arch_round_mon_val(threshold);
>  
> -	ret = dom_data_init(r);
> -	if (ret)
> -		return ret;
> -
>  	if (rdt_cpu_has(X86_FEATURE_BMEC)) {
>  		u32 eax, ebx, ecx, edx;
>  
> @@ -1240,6 +1271,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  		}
>  	}
>  
> +	ret = dom_data_init(r);
> +	if (ret)
> +		return ret;
> +
>  	l3_mon_evt_init(r);
>  
>  	r->mon_capable = true;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c48b5450e6c2..8ffebd203c31 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -185,6 +185,25 @@ bool closid_allocated(unsigned int closid)
>  	return !test_bit(closid, &closid_free_map);
>  }
>  
> +int mbm_cntr_alloc(struct rdt_resource *r)
> +{
> +	int cntr_id;
> +
> +	cntr_id = find_first_bit(r->mon.mbm_cntr_free_map,
> +				 r->mon.num_mbm_cntrs);
> +	if (cntr_id >= r->mon.num_mbm_cntrs)
> +		return -ENOSPC;
> +
> +	__clear_bit(cntr_id, r->mon.mbm_cntr_free_map);
> +
> +	return cntr_id;
> +}
> +
> +void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id)
> +{
> +	__set_bit(cntr_id, r->mon.mbm_cntr_free_map);
> +}
> +
>  /**
>   * rdtgroup_mode_by_closid - Return mode of resource group with closid
>   * @closid: closid if the resource group
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f11d6fdfd977..5a4d6adec974 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -187,12 +187,14 @@ enum resctrl_scope {
>   * @num_rmid:		Number of RMIDs available
>   * @num_mbm_cntrs:	Number of assignable monitoring counters
>   * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?
> + * @mbm_cntr_free_map:	bitmap of free MBM counters
>   * @evt_list:		List of monitoring events
>   */

Please follow custom of existing doc and have description start with capital letter.

>  struct resctrl_mon {
>  	int			num_rmid;
>  	int			num_mbm_cntrs;
>  	bool			mbm_cntr_assignable;
> +	unsigned long		*mbm_cntr_free_map;
>  	struct list_head	evt_list;
>  };
>  

Reinette