[PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters

Babu Moger posted 24 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Babu Moger 1 year, 3 months ago
Hardware provides a set of counters when mbm_cntr_assignable feature is
supported. These counters are used for assigning the events in resctrl
a group when the feature is enabled. The kernel must manage and track the
number of available counters.

Introduce mbm_cntr_free_map bitmap to track available counters and set
of routines to allocate and free the counters.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v7: Removed the static allocation and now allocating bitmap mbm_cntr_free_map
    dynamically.
    Passed the struct rdt_resource mbm_cntr_alloc and mbm_cntr_free.
    Removed the reference of ABMC and changed it mbm_cntr_assign.
    Few other text changes.

v6: Removed the variable mbm_cntrs_free_map_len. This is not required.
    Removed the call mbm_cntrs_init() in arch code. This needs to be
    done at higher level.
    Used DECLARE_BITMAP to initialize mbm_cntrs_free_map.
    Moved all the counter interfaces mbm_cntr_alloc() and mbm_cntr_free()
    in here as part of separating arch and fs bits.

v5:
   Updated the comments and commit log.
   Few renames
    num_cntrs_free_map -> mbm_cntrs_free_map
    num_cntrs_init -> mbm_cntrs_init
    Added initialization in rdt_get_tree because the default ABMC
    enablement happens during the init.

v4: Changed the name to num_cntrs where applicable.
     Used bitmap apis.
     Added more comments for the globals.

v3: Changed the bitmap name to assign_cntrs_free_map. Removed abmc
     from the name.

v2: Changed the bitmap name to assignable_counter_free_map from
     abmc_counter_free_map.
---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 +-
 arch/x86/kernel/cpu/resctrl/internal.h |  4 +++-
 arch/x86/kernel/cpu/resctrl/monitor.c  | 31 +++++++++++++++++++++++++-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 ++++++++++++++++
 include/linux/resctrl.h                |  2 ++
 5 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 49d147e2e4e5..00ad00258df2 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -1140,7 +1140,7 @@ static void __exit resctrl_exit(void)
 	rdtgroup_exit();
 
 	if (r->mon_capable)
-		rdt_put_mon_l3_config();
+		rdt_put_mon_l3_config(r);
 }
 
 __exitcall(resctrl_exit);
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index a45ae410274c..99f9103a35ba 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -633,7 +633,7 @@ void closid_free(int closid);
 int alloc_rmid(u32 closid);
 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);
+void __exit rdt_put_mon_l3_config(struct rdt_resource *r);
 bool __init rdt_cpu_has(int flag);
 void mon_event_count(void *info);
 int rdtgroup_mondata_show(struct seq_file *m, void *arg);
@@ -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 e3e71843401a..f98cc5b9bebc 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1175,6 +1175,30 @@ static __init int snc_get_config(void)
 	return ret;
 }
 
+/*
+ * 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.
+ * Kernel needs to keep track of the number of available counters.
+ */
+static int mbm_cntrs_init(struct rdt_resource *r)
+{
+	if (r->mon.mbm_cntr_assignable) {
+		r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
+							 GFP_KERNEL);
+		if (!r->mon.mbm_cntr_free_map)
+			return -ENOMEM;
+		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
+	}
+	return 0;
+}
+
+static void __exit mbm_cntrs_exit(struct rdt_resource *r)
+{
+	if (r->mon.mbm_cntr_assignable)
+		bitmap_free(r->mon.mbm_cntr_free_map);
+}
+
 int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 {
 	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
@@ -1240,6 +1264,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 		}
 	}
 
+	ret = mbm_cntrs_init(r);
+	if (ret)
+		return ret;
+
 	l3_mon_evt_init(r);
 
 	r->mon_capable = true;
@@ -1247,9 +1275,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	return 0;
 }
 
-void __exit rdt_put_mon_l3_config(void)
+void __exit rdt_put_mon_l3_config(struct rdt_resource *r)
 {
 	dom_data_exit();
+	mbm_cntrs_exit(r);
 }
 
 void __init intel_rdt_mbm_apply_quirk(void)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index ba737890d5c2..a51992984832 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..aab22ff8e0c1 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 number of assignable MBM counters
  * @evt_list:		List of monitoring events
  */
 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;
 };
 
-- 
2.34.1
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Peter Newman 1 year, 2 months ago
Hi Babu,

On Wed, Sep 4, 2024 at 3:23 PM Babu Moger <babu.moger@amd.com> wrote:

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f11d6fdfd977..aab22ff8e0c1 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 number of assignable MBM counters
>   * @evt_list:          List of monitoring events
>   */
>  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;
>  };

This looks global still. Will only all-domain (*=) operations be
supported initially?

Thanks,
-Peter
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Moger, Babu 1 year, 2 months ago
Hi Peter,

On 9/24/24 11:25, Peter Newman wrote:
> Hi Babu,
> 
> On Wed, Sep 4, 2024 at 3:23 PM Babu Moger <babu.moger@amd.com> wrote:
> 
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index f11d6fdfd977..aab22ff8e0c1 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 number of assignable MBM counters
>>   * @evt_list:          List of monitoring events
>>   */
>>  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;
>>  };
> 
> This looks global still. Will only all-domain (*=) operations be
> supported initially?

Yes. It is supported in this series.

We have one counter at global level and another at domain level.
https://lore.kernel.org/lkml/7a24bb182897acab3daaac1cadaabca3bcc73dc5.1725488488.git.babu.moger@amd.com/

Domain level counter is used for tracking the counters status in each domain.

Global counter is released once the counter is freed in all the domains.

-- 
Thanks
Babu Moger
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Reinette Chatre 1 year, 3 months ago
Hi Babu,

On 9/4/24 3:21 PM, Babu Moger wrote:
> Hardware provides a set of counters when mbm_cntr_assignable feature is
> supported. These counters are used for assigning the events in resctrl
> a group when the feature is enabled. The kernel must manage and track the

The second sentence ("These counters ...") is difficult to parse.

> number of available counters.

"The kernel must manage and track the number of available counters." ->
"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.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index e3e71843401a..f98cc5b9bebc 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1175,6 +1175,30 @@ static __init int snc_get_config(void)
>  	return ret;
>  }
>  
> +/*
> + * 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.

(soft-ABMC may need to edit this comment)

> + * Kernel needs to keep track of the number of available counters.

Last sentence seems to be duplicate of the first?

> + */
> +static int mbm_cntrs_init(struct rdt_resource *r)

Needs __init?

> +{
> +	if (r->mon.mbm_cntr_assignable) {
> +		r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
> +							 GFP_KERNEL);
> +		if (!r->mon.mbm_cntr_free_map)
> +			return -ENOMEM;
> +		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
> +	}
> +	return 0;
> +}
> +
> +static void __exit mbm_cntrs_exit(struct rdt_resource *r)
> +{
> +	if (r->mon.mbm_cntr_assignable)
> +		bitmap_free(r->mon.mbm_cntr_free_map);
> +}
> +
>  int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  {
>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> @@ -1240,6 +1264,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  		}
>  	}
>  
> +	ret = mbm_cntrs_init(r);
> +	if (ret)
> +		return ret;

Missing cleanup of earlier allocation on error path here. Even so, this does not
seem to integrate with existing dom_data_init() wrt ordering and locking. Could
this be more fitting when merged with dom_data_init() (after moving it)?

> +
>  	l3_mon_evt_init(r);
>  
>  	r->mon_capable = true;
> @@ -1247,9 +1275,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  	return 0;
>  }
>  
> -void __exit rdt_put_mon_l3_config(void)
> +void __exit rdt_put_mon_l3_config(struct rdt_resource *r)
>  {
>  	dom_data_exit();
> +	mbm_cntrs_exit(r);

There is a mismatch wrt locking used in dom_data_exit() and mbm_cntrs_exit() that is
sure to cause confusion and difficulty in the MPAM transition.

>  }
>  
>  void __init intel_rdt_mbm_apply_quirk(void)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ba737890d5c2..a51992984832 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..aab22ff8e0c1 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 number of assignable MBM counters

The "number of" is not clear ... it seems to indicate tracking a count? How about
just "bitmap of free MBM counters"

>   * @evt_list:		List of monitoring events
>   */
>  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
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Moger, Babu 1 year, 2 months ago
Hi Reinette,

On 9/19/24 11:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/4/24 3:21 PM, Babu Moger wrote:
>> Hardware provides a set of counters when mbm_cntr_assignable feature is
>> supported. These counters are used for assigning the events in resctrl
>> a group when the feature is enabled. The kernel must manage and track the
> 
> The second sentence ("These counters ...") is difficult to parse.

How about?

Counters are used for assigning the events in resctrl group.

> 
>> number of available counters.
> 
> "The kernel must manage and track the number of available counters." ->
> "The kernel must manage and track the available counters." ?

Sure.

> 
>>
>> Introduce mbm_cntr_free_map bitmap to track available counters and set
>> of routines to allocate and free the counters.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index e3e71843401a..f98cc5b9bebc 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1175,6 +1175,30 @@ static __init int snc_get_config(void)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * 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.
> 
> (soft-ABMC may need to edit this comment)

Agree..

> 
>> + * Kernel needs to keep track of the number of available counters.
> 
> Last sentence seems to be duplicate of the first?

Will remove it.

> 
>> + */
>> +static int mbm_cntrs_init(struct rdt_resource *r)
> 
> Needs __init?

Did you mean to merge this with dom_data_init and don't have to have a
separate function. Please clarify.


> 
>> +{
>> +	if (r->mon.mbm_cntr_assignable) {
>> +		r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
>> +							 GFP_KERNEL);
>> +		if (!r->mon.mbm_cntr_free_map)
>> +			return -ENOMEM;
>> +		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void __exit mbm_cntrs_exit(struct rdt_resource *r)
>> +{
>> +	if (r->mon.mbm_cntr_assignable)
>> +		bitmap_free(r->mon.mbm_cntr_free_map);
>> +}
>> +
>>  int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  {
>>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>> @@ -1240,6 +1264,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  		}
>>  	}
>>  
>> +	ret = mbm_cntrs_init(r);
>> +	if (ret)
>> +		return ret;
> 
> Missing cleanup of earlier allocation on error path here. Even so, this does not
> seem to integrate with existing dom_data_init() wrt ordering and locking. Could
> this be more fitting when merged with dom_data_init() (after moving it)?

Sure. Will do.

> 
>> +
>>  	l3_mon_evt_init(r);
>>  
>>  	r->mon_capable = true;
>> @@ -1247,9 +1275,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  	return 0;
>>  }
>>  
>> -void __exit rdt_put_mon_l3_config(void)
>> +void __exit rdt_put_mon_l3_config(struct rdt_resource *r)
>>  {
>>  	dom_data_exit();
>> +	mbm_cntrs_exit(r);
> 
> There is a mismatch wrt locking used in dom_data_exit() and mbm_cntrs_exit() that is
> sure to cause confusion and difficulty in the MPAM transition.

Will merge this with dom_data_exit.

> 
>>  }
>>  
>>  void __init intel_rdt_mbm_apply_quirk(void)
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index ba737890d5c2..a51992984832 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..aab22ff8e0c1 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 number of assignable MBM counters
> 
> The "number of" is not clear ... it seems to indicate tracking a count? How about
> just "bitmap of free MBM counters"

Sure.

> 
>>   * @evt_list:		List of monitoring events
>>   */
>>  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
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Reinette Chatre 1 year, 2 months ago
Hi Babu,

On 9/23/24 11:33 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/19/24 11:42, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>> Hardware provides a set of counters when mbm_cntr_assignable feature is
>>> supported. These counters are used for assigning the events in resctrl
>>> a group when the feature is enabled. The kernel must manage and track the
>>
>> The second sentence ("These counters ...") is difficult to parse.
> 
> How about?
> 
> Counters are used for assigning the events in resctrl group.

Apologies but I am just not able to parse this. How about: "These counters
are assigned to the MBM monitoring events of a MON group that needs to
be tracked."

...

>>> + */
>>> +static int mbm_cntrs_init(struct rdt_resource *r)
>>
>> Needs __init?
> 
> Did you mean to merge this with dom_data_init and don't have to have a
> separate function. Please clarify.

Here I was referring to the actual __init storage class attribute. Since
mbm_cntrs_init() is only called by __init code, it too should have the
__init storage class attribute.
I do expect that mbm_cntrs_init() will be called by dom_data_init() and
care should be taken when making this change since it seems that dom_data_init()
itself needs the __init storage class attribute. Looks like this was missed
by commit bd334c86b5d7 ("x86/resctrl: Add __init attribute to rdt_get_mon_l3_config()")

Reinette
Re: [PATCH v7 09/24] x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable counters
Posted by Moger, Babu 1 year, 2 months ago
Hi Reinette,

On 9/23/24 17:28, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/23/24 11:33 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 9/19/24 11:42, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>> Hardware provides a set of counters when mbm_cntr_assignable feature is
>>>> supported. These counters are used for assigning the events in resctrl
>>>> a group when the feature is enabled. The kernel must manage and track the
>>>
>>> The second sentence ("These counters ...") is difficult to parse.
>>
>> How about?
>>
>> Counters are used for assigning the events in resctrl group.
> 
> Apologies but I am just not able to parse this. How about: "These counters
> are assigned to the MBM monitoring events of a MON group that needs to
> be tracked."

Sure.

> ...
> 
>>>> + */
>>>> +static int mbm_cntrs_init(struct rdt_resource *r)
>>>
>>> Needs __init?
>>
>> Did you mean to merge this with dom_data_init and don't have to have a
>> separate function. Please clarify.
> 
> Here I was referring to the actual __init storage class attribute. Since
> mbm_cntrs_init() is only called by __init code, it too should have the
> __init storage class attribute.
> I do expect that mbm_cntrs_init() will be called by dom_data_init() and
> care should be taken when making this change since it seems that dom_data_init()
> itself needs the __init storage class attribute. Looks like this was missed
> by commit bd334c86b5d7 ("x86/resctrl: Add __init attribute to rdt_get_mon_l3_config()")

Sure. I can take care of this.

-- 
Thanks
Babu Moger