Re: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details

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

Hi Reinette,

On 5/3/24 18:26, Reinette Chatre wrote:
> Hi Babu,
> 
> On 3/28/2024 6:06 PM, Babu Moger wrote:
>> ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5.
>> Bits Description
>> 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
>>      Monitoring Counter ID + 1
>>
>> The feature details are documented in APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
>> Monitoring (ABMC).
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> v3: Removed changes related to mon_features.
>>     Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc.
>>     Also moved the fields mbm_assign_capable and mbm_assign_cntrs to
>>     rdt_resource. (James)
>>
>> v2: Changed the field name to mbm_assign_capable from abmc_capable.
>> ---
>>  arch/x86/kernel/cpu/resctrl/core.c     | 17 +++++++++++++++++
>>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>>  arch/x86/kernel/cpu/resctrl/monitor.c  |  3 +++
>>  include/linux/resctrl.h                | 12 ++++++++++++
>>  4 files changed, 33 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 57a8c6f30dd6..bb82b392cf5d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -740,6 +740,23 @@ bool __init rdt_cpu_has(int flag)
>>  	return ret;
>>  }
>>  
>> +inline bool __init resctrl_arch_has_abmc(struct rdt_resource *r)
>> +{
>> +	bool ret = rdt_cpu_has(X86_FEATURE_ABMC);
>> +	u32 eax, ebx, ecx, edx;
>> +
>> +	if (ret) {
>> +		/*
>> +		 * Query CPUID_Fn80000020_EBX_x05 for number of
>> +		 * ABMC counters
>> +		 */
>> +		cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>> +		r->mbm_assign_cntrs = (ebx & 0xFFFF) + 1;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> It is not clear to me why this function is needed. I went back to
> read James' comment and it sounds to me as though he expected it
> to be called from non-arch code ... but this is only called
> from rdt_get_mon_l3_config() which is very much architecture specific
> and will remain in arch/x86 where rdt_cpu_has() will be accessible.

Yes. That is correct.  I will revert it and move it to rdt_get_mon_l3_config.

> 
>> +
>>  static __init bool get_mem_config(void)
>>  {
>>  	struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_MBA];
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c99f26ebe7a6..c4ae6f3993aa 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -584,6 +584,7 @@ void free_rmid(u32 closid, u32 rmid);
>>  int rdt_get_mon_l3_config(struct rdt_resource *r);
>>  void __exit rdt_put_mon_l3_config(void);
>>  bool __init rdt_cpu_has(int flag);
>> +bool __init resctrl_arch_has_abmc(struct rdt_resource *r);
>>  void mon_event_count(void *info);
>>  int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index c34a35ec0f03..e5938bf53d5a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1055,6 +1055,9 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  			mbm_local_event.configurable = true;
>>  			mbm_config_rftype_init("mbm_local_bytes_config");
>>  		}
>> +
>> +		if (resctrl_arch_has_abmc(r))
>> +			r->mbm_assign_capable = ABMC_ASSIGN;
>>  	}
> 
> This is confusing to me in two ways:
> (a) why need different layers of abstraction to initialize r->mbm_assign_capable
>     and r->mbm_assign_cntrs? Can they not just be assigned at the same time?

Yes. we can.

> (b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
>     this enum needed for this?

Enum is really not required. Will correct it.

> 
>>  
>>  	l3_mon_evt_init(r);
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index a365f67131ec..a1ee9afabff3 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -150,6 +150,14 @@ struct resctrl_membw {
>>  struct rdt_parse_data;
>>  struct resctrl_schema;
>>  
>> +/**
>> + * enum mbm_assign_type - The type of assignable monitoring.
>> + * @ABMC_ASSIGN: Assignable Bandwidth Monitoring Counters.
>> + */
>> +enum mbm_assign_type {
>> +	ABMC_ASSIGN	= 0x01,
>> +};
>> +
> 
> Either the resource is mbm_assign_capable or not ... it is not clear
> to me why an enum is needed.

This is not required.

> 
>>  /**
>>   * struct rdt_resource - attributes of a resctrl resource
>>   * @rid:		The index of the resource
>> @@ -168,6 +176,8 @@ struct resctrl_schema;
>>   * @evt_list:		List of monitoring events
>>   * @fflags:		flags to choose base and info files
>>   * @cdp_capable:	Is the CDP feature available on this resource
>> + * @mbm_assign_capable:	Does system capable of supporting monitor assignment?
> 
> "Does system capable" -> "Is system capable"?

Sure.

> 
>> + * @mbm_assign_cntrs:	Maximum number of assignable counters
>>   */
>>  struct rdt_resource {
>>  	int			rid;
>> @@ -188,6 +198,8 @@ struct rdt_resource {
>>  	struct list_head	evt_list;
>>  	unsigned long		fflags;
>>  	bool			cdp_capable;
>> +	bool			mbm_assign_capable;
>> +	u32                     mbm_assign_cntrs;
>>  };
> 
> Please check tabs vs spaces (in this whole series please).

Sure. Will do.

> 
> I'm thinking that a new "MBM specific" struct within
> struct rdt_resource will be helpful to clearly separate the MBM related
> data. This will be similar to struct resctrl_cache for 
> cache allocation and struct resctrl_membw for memory bandwidth
> allocation.

Did you mean to move all the fields for monitoring to move to new struct?

Struct resctrl_mbm {
          int                     num_rmid;
          bool                    mbm_assign_capable;
          u32                     mbm_assign_cntrs;
}:

-- 
Thanks
Babu Moger