linux-kernel - Re: [PATCH v4 09/19] x86/resctrl: Initialize ABMC counters bitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <915bebc0-03bb-453f-b468-6cd8aec08de5@intel.com>
Date: Thu, 13 Jun 2024 18:42:15 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <fenghua.yu@...el.com>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
	<rdunlap@...radead.org>, <tj@...nel.org>, <peterz@...radead.org>,
	<yanjiewtw@...il.com>, <kim.phillips@....com>, <lukas.bulwahn@...il.com>,
	<seanjc@...gle.com>, <jmattson@...gle.com>, <leitao@...ian.org>,
	<jpoimboe@...nel.org>, <rick.p.edgecombe@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <jithu.joseph@...el.com>,
	<kai.huang@...el.com>, <kan.liang@...ux.intel.com>,
	<daniel.sneddon@...ux.intel.com>, <pbonzini@...hat.com>,
	<sandipan.das@....com>, <ilpo.jarvinen@...ux.intel.com>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<eranian@...gle.com>, <james.morse@....com>
Subject: Re: [PATCH v4 09/19] x86/resctrl: Initialize ABMC counters bitmap

Hi Babu,

On 5/24/24 5:23 AM, 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 supported.
> 
> Introduce num_cntrs_free_map bitmap to track available and free counters.
> 
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> 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 | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3071bbb7a15e..400ae405e10e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
>   	return !test_bit(closid, &closid_free_map);
>   }
>   
> +/*
> + * ABMC Counter bitmap and 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
> + * of number of available counters.
> + */
> +static unsigned long num_cntrs_free_map;

Why does variable have "num" in its name? That seems strange. How
about just "mon_cntrs_free_map

> +static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

> +
> +static void num_cntrs_init(void)

mon_cntrs_init() ?

> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +
> +	bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
> +	num_cntrs_free_map_len = r->mon.num_cntrs;
> +}
> +
>   /**
>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>    * @closid: closid if the resource group
> @@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum resctrl_res_level l, bool enable)

resctrl_abmc_set_all() was initially created as a complement of
resctrl_abmc_set_one() ... but with more initialization added to
resctrl_abmc_set_all() this relationship becomes vague.

>   	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>   	struct rdt_domain *d;
>   
> +	/*
> +	 * The bitmap num_cntrs_free_map needs to be reset when switching
> +	 * the monitor mode.

This comment puts in words what can be understood by just looking at the code.
What would be useful would be to explain _why_ this needs to be done ...
what happens to the previously assigned counters?


> +	 */
> +	num_cntrs_init();
> +
>   	/*
>   	 * Update QOS_CFG MSR on all the CPUs associated with the resource
>   	 * Hardware counters will reset after switching the monotor mode.

Reinette