Re: [PATCH v5 09/20] x86/resctrl: Initialize monitor counters bitmap

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

Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> Hardware provides a set of counters when the ABMC feature is supported.
> These counters are used for enabling the events in resctrl group when
> the feature is enabled.
> 
> Introduce mbm_cntrs_free_map bitmap to track available and free counters.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> 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/rdtgroup.c | 29 ++++++++++++++++++++++++--
>   1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 4f47f52e01c2..b3d3fa048f15 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
>   	return !test_bit(closid, &closid_free_map);
>   }
>   
> +/*
> + * Counter bitmap and its length for tracking available counters.
> + * ABMC feature provides set of hardware counters for enabling events.
> + * Each event takes one hardware counter. Kernel needs to keep track

What is meant with "Kernel" here? It looks to be the fs code but the
implementation has both fs and arch code reaching into the counter
management. This should not be the case, either the fs code or the
arch code needs to manage the counters, not both.

> + * of number of available counters.
> + */
> +static unsigned long mbm_cntrs_free_map;

With the lengths involved this needs a proper DECLARE_BITMAP()

> +static unsigned int mbm_cntrs_free_map_len;
> +
> +static void mbm_cntrs_init(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +
> +	bitmap_fill(&mbm_cntrs_free_map, r->mon.num_mbm_cntrs);
> +	mbm_cntrs_free_map_len = r->mon.num_mbm_cntrs;
> +}
> +
>   /**
>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>    * @closid: closid if the resource group
> @@ -2466,6 +2483,12 @@ static int _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
>   {
>   	struct rdt_mon_domain *d;
>   
> +	/*
> +	 * Clear all the previous assignments while switching the monitor
> +	 * mode.
> +	 */
> +	mbm_cntrs_init();
> +

If the counters are managed by fs code then the arch code should not be
doing this. If needed the fs code should init the counters before calling
the arch helpers.

>   	/*
>   	 * Hardware counters will reset after switching the monitor mode.
>   	 * Reset the architectural state so that reading of hardware
> @@ -2724,10 +2747,10 @@ static void schemata_list_destroy(void)
>   
>   static int rdt_get_tree(struct fs_context *fc)
>   {
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>   	struct rdt_fs_context *ctx = rdt_fc2context(fc);
>   	unsigned long flags = RFTYPE_CTRL_BASE;
>   	struct rdt_mon_domain *dom;
> -	struct rdt_resource *r;
>   	int ret;
>   
>   	cpus_read_lock();
> @@ -2756,6 +2779,9 @@ static int rdt_get_tree(struct fs_context *fc)
>   
>   	closid_init();
>   
> +	if (r->mon.abmc_capable)
> +		mbm_cntrs_init();
> +
>   	if (resctrl_arch_mon_capable())
>   		flags |= RFTYPE_MON;
>   
> @@ -2800,7 +2826,6 @@ static int rdt_get_tree(struct fs_context *fc)
>   		resctrl_mounted = true;
>   
>   	if (is_mbm_enabled()) {
> -		r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>   		list_for_each_entry(dom, &r->mon_domains, hdr.list)
>   			mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL,
>   						   RESCTRL_PICK_ANY_CPU);

Reinette