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;
+}
+
 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;
 	}
 
 	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,
+};
+
 /**
  * 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?
+ * @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;
 };
 
 /**
-- 
2.34.1

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.

> +
>  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?
(b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
    this enum needed for this?

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

>  /**
>   * 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"?

> + * @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).

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.

>  
>  /**

Reinette

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

Hi Babu,

On 5/6/2024 12:09 PM, Moger, Babu wrote:
> On 5/3/24 18:26, Reinette Chatre wrote:
>> On 3/28/2024 6:06 PM, Babu Moger wrote:

...

>>> + * @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;
> }:
> 

Indeed, so not actually MBM specific but monitoring specific as you state (with
appropriate naming?). This is purely to help organize data within struct rdt_resource
and (similar to struct resctrl_cache and struct resctrl_membw) not a new
structure expected to be passed around. I think evt_list can also be a member.

Reinette

Hi Reinette,

On 5/7/24 15:27, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>> On 5/3/24 18:26, Reinette Chatre wrote:
>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
> 
> ...
> 
>>>> + * @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;
>> }:
>>
> 
> Indeed, so not actually MBM specific but monitoring specific as you state (with
> appropriate naming?). This is purely to help organize data within struct rdt_resource
> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
> structure expected to be passed around. I think evt_list can also be a member.

How about this?

Lets keep assign_capable in main structure(like we have mon_capable) and
move rest of them into new structure.

Struct resctrl_mon {
           int                     num_rmid;
	   struct list_head        evt_list;
           u32                     num_assign_cntrs;
}:
-- 
Thanks
Babu Moger

Hi Babu,

On 5/9/2024 3:34 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 5/7/24 15:27, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>>> On 5/3/24 18:26, Reinette Chatre wrote:
>>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>>
>> ...
>>
>>>>> + * @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;
>>> }:
>>>
>>
>> Indeed, so not actually MBM specific but monitoring specific as you state (with
>> appropriate naming?). This is purely to help organize data within struct rdt_resource
>> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
>> structure expected to be passed around. I think evt_list can also be a member.
> 
> How about this?
> 
> Lets keep assign_capable in main structure(like we have mon_capable) and
> move rest of them into new structure.
> 
> Struct resctrl_mon {
>            int                     num_rmid;
> 	   struct list_head        evt_list;
>            u32                     num_assign_cntrs;
> }:

This looks like a good start. It certainly supports ABMC. I do not yet
have a clear understanding about how this will support MPAM and soft-RMID/ABMC
since the current implementation has an implicit scope of one counter per event per
monitor group. It thus seem reasonable to have a new generic name of "cntrs".

Reinette

Hi Reinette,

On 5/9/2024 10:18 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/9/2024 3:34 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 5/7/24 15:27, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>>>> On 5/3/24 18:26, Reinette Chatre wrote:
>>>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>>>> + * @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;
>>>> }:
>>>>
>>>
>>> Indeed, so not actually MBM specific but monitoring specific as you state (with
>>> appropriate naming?). This is purely to help organize data within struct rdt_resource
>>> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
>>> structure expected to be passed around. I think evt_list can also be a member.
>>
>> How about this?
>>
>> Lets keep assign_capable in main structure(like we have mon_capable) and
>> move rest of them into new structure.
>>
>> Struct resctrl_mon {
>>             int                     num_rmid;
>> 	   struct list_head        evt_list;
>>             u32                     num_assign_cntrs;
>> }:
> 
> This looks like a good start. It certainly supports ABMC. I do not yet
> have a clear understanding about how this will support MPAM and soft-RMID/ABMC
> since the current implementation has an implicit scope of one counter per event per
> monitor group. It thus seem reasonable to have a new generic name of "cntrs".

How about renaming it to "assignable_counters"?

-- 
- Babu Moger

Hi Babu,

On 5/10/2024 10:01 AM, Moger, Babu wrote:
> On 5/9/2024 10:18 PM, Reinette Chatre wrote:
>> On 5/9/2024 3:34 PM, Moger, Babu wrote:
>>> On 5/7/24 15:27, Reinette Chatre wrote:
>>>> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>>>>> On 5/3/24 18:26, Reinette Chatre wrote:
>>>>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>>>>
>>>> ...
>>>>
>>>>>>> + * @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;
>>>>> }:
>>>>>
>>>>
>>>> Indeed, so not actually MBM specific but monitoring specific as you state (with
>>>> appropriate naming?). This is purely to help organize data within struct rdt_resource
>>>> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
>>>> structure expected to be passed around. I think evt_list can also be a member.
>>>
>>> How about this?
>>>
>>> Lets keep assign_capable in main structure(like we have mon_capable) and
>>> move rest of them into new structure.
>>>
>>> Struct resctrl_mon {
>>>             int                     num_rmid;
>>>        struct list_head        evt_list;
>>>             u32                     num_assign_cntrs;
>>> }:
>>
>> This looks like a good start. It certainly supports ABMC. I do not yet
>> have a clear understanding about how this will support MPAM and soft-RMID/ABMC
>> since the current implementation has an implicit scope of one counter per event per
>> monitor group. It thus seem reasonable to have a new generic name of "cntrs".
> 
> How about renaming it to "assignable_counters"?
> 

As I mentioned in [1] the "assign" concept is not clear (just to me perhaps?). It
really seems to be the marketing name percolating into the implementation. Why is
"assignable" needed to be in a "counter" variable name? Is a variable not by definition
"assignable"? Why not just, for example, "num_cntrs"? I believe that things to be
as simple and obvious as possible ... this just helps to reduce confusion.

My previous comment regarding counter scope is not addressed by a rename though and
I do not expect you to have the answer but I would like us to keep in mind how these
"counters" could end up getting used. We may just later, when soft-RMID/ABMC and MPAM
is understood, need to add a "counter scope" as well.

Reinette

[1] https://lore.kernel.org/lkml/0d94849c-828c-4f10-a6f8-e26cc4554909@intel.com/

Hi Reinette,


On 5/10/2024 1:34 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/10/2024 10:01 AM, Moger, Babu wrote:
>> On 5/9/2024 10:18 PM, Reinette Chatre wrote:
>>> On 5/9/2024 3:34 PM, Moger, Babu wrote:
>>>> On 5/7/24 15:27, Reinette Chatre wrote:
>>>>> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>>>>>> On 5/3/24 18:26, Reinette Chatre wrote:
>>>>>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>> + * @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;
>>>>>> }:
>>>>>>
>>>>>
>>>>> Indeed, so not actually MBM specific but monitoring specific as you state (with
>>>>> appropriate naming?). This is purely to help organize data within struct rdt_resource
>>>>> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
>>>>> structure expected to be passed around. I think evt_list can also be a member.
>>>>
>>>> How about this?
>>>>
>>>> Lets keep assign_capable in main structure(like we have mon_capable) and
>>>> move rest of them into new structure.
>>>>
>>>> Struct resctrl_mon {
>>>>              int                     num_rmid;
>>>>         struct list_head        evt_list;
>>>>              u32                     num_assign_cntrs;
>>>> }:
>>>
>>> This looks like a good start. It certainly supports ABMC. I do not yet
>>> have a clear understanding about how this will support MPAM and soft-RMID/ABMC
>>> since the current implementation has an implicit scope of one counter per event per
>>> monitor group. It thus seem reasonable to have a new generic name of "cntrs".
>>
>> How about renaming it to "assignable_counters"?
>>
> 
> As I mentioned in [1] the "assign" concept is not clear (just to me perhaps?). It
> really seems to be the marketing name percolating into the implementation. Why is
> "assignable" needed to be in a "counter" variable name? Is a variable not by definition
> "assignable"? Why not just, for example, "num_cntrs"? I believe that things to be
> as simple and obvious as possible ... this just helps to reduce confusion.

Ok. That sounds fine to me.
> 
> My previous comment regarding counter scope is not addressed by a rename though and
> I do not expect you to have the answer but I would like us to keep in mind how these
> "counters" could end up getting used. We may just later, when soft-RMID/ABMC and MPAM
> is understood, need to add a "counter scope" as well.

Sure.

> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/0d94849c-828c-4f10-a6f8-e26cc4554909@intel.com/
> 

Thanks
- Babu Moger