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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 10/15/24 22:14, Reinette Chatre wrote:
> 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*.

Will change it to.

dom_data_init() requires mbm_cntr_assign state to initialize
mbm_cntr_free_map bitmap. Move dom_data_init() after mbm_cntr_assign
detection.

> 
>>
>> 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.


Sure.

> 
>> +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.

Sure.

> 
>>  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
> 

-- 
Thanks
Babu Moger