Received-SPF: pass (google.com: domain of linux-kernel+bounces-168412-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <aec3fdfa-ad57-402b-9c21-555b5ddc85f9@intel.com>
Date: Fri, 3 May 2024 16:31:23 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 08/17] x86/resctrl: Initialize assignable counters
 bitmap
To: Babu Moger <babu.moger@amd.com>, <corbet@lwn.net>, <fenghua.yu@intel.com>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<dave.hansen@linux.intel.com>
CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<yanjiewtw@gmail.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>,
	<seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>,
	<jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>,
	<kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>,
	<kai.huang@intel.com>, <kan.liang@linux.intel.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>,
	<peternewman@google.com>, <maciej.wieczor-retman@intel.com>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<eranian@google.com>, <james.morse@arm.com>
References: <cover.1711674410.git.babu.moger@amd.com>
 <d9fe6638d901791dfdec26caed034179c636e17b.1711674410.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <d9fe6638d901791dfdec26caed034179c636e17b.1711674410.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QndqUjVtb3lVWFFpczdycVpWeUd1U084VTRncUJYTFVkS0RLTksvMFpIUGNu?=
 =?utf-8?B?amt4bjl1NysxUnpaVW4vUllJbGpZT3VmdlZZUXFjTWFodzVFeTREb2ErN0V2?=
 =?utf-8?B?aUU2WmhzampTb3BWVm5nRkE1cHRxRGwxdGppcWZYRHZFYkpLQmZXUFFKYzFy?=
 =?utf-8?B?cjhCcVhjajl4eTB2K2NpRU5kOS9ZVUhUTHd2ZlYvdVlxTEthNVVkNG5ZQnQ3?=
 =?utf-8?B?Q1pHczVFQlpCaXdwYTVjUnl4NE9LNmM0U3hacW1BSk4xUVhDbm42QlBkZjhi?=
 =?utf-8?B?NEtNRHlvVy9PMms4Zmg5a0pHZVkwZmxyUS8rNzh4NzNaa3hQQUhZWm5DR29l?=
 =?utf-8?B?a25EQlVyLzNkYzhRK1RLNkYzUWJySzM2by8zcXlyMnZVdzhUN2h3ZnNQK1Rz?=
 =?utf-8?B?KzZuS29LZHVhMlRTODZFRld5STk2Q25rTnZPNHFyVjRrWHc2NlNYWTNCZk55?=
 =?utf-8?B?TDZqaStNa3FLQzNCWm5tZml5dldoWW5NdERHcnRPajhjOEtiY0N6dWpicjJt?=
 =?utf-8?B?TTNVUHFRT2Q5WWwzZTRPMGpLMHJzVlJmZHdLSXJ6cDVYVHo4cGRhNGNkUzFK?=
 =?utf-8?B?ZWFsTUJoa3pTTjhkTWwzV0J0KzhMOWdTemYrS2oxU1ZYSytFTE5zMFVjZUli?=
 =?utf-8?B?MkxiQlZrTUNaUS9nNHdCZy80RzB6SEljRXB3QUZObFhHQSs5VEhpTUk4NzRi?=
 =?utf-8?B?aFRXRk4rUC9WcG5sTjFyNFhtcjUreHZZaVFzUWFUdzRHZk9yMzIwOFo4UXdE?=
 =?utf-8?B?L01aM0J2dEUvelRGYktlWFIxUVRVUWhLRkxuUEZBYjRsckJEL0I5YlQzT2pH?=
 =?utf-8?B?TmFHLzluS0xFN0VGOEQ1aGp0T1NrNXMvSnNRN2FGOWcyV2VRR0xZVE56Q2ps?=
 =?utf-8?B?TXhiUE9YRmo0c3lCcFBxUWtJenk0d1pZWUhIVjNvUFgxUnhua1puYjlLQm5V?=
 =?utf-8?B?M3J1WSt2a3VIYUZIV0prWVpZUGxiUEZaYk5TL2Zjb0V0UGQ0a3N6U3N4TklX?=
 =?utf-8?B?Mk1JbzZveXBUbHRkM0tJMVUxaVFyUzdxb2R5UG50VmQ3OUpVaDFhSFpnWjFV?=
 =?utf-8?B?akZ2YURxUmdkbTBJZ1pCQzlZc0w1ODNONXdPVkRSSFlFODd6Vk1ZSXhQR0Z0?=
 =?utf-8?B?elVwUk1pT0JpUU1Iak5LaDk3YU1IdmtYVFVyT2JqKzN4WTVOcDlMbm1FNkoz?=
 =?utf-8?B?K3NCZ2V2S2JHM2xld2JzQ203ckFSVHBFc0l0Znd1OHY2aytHOHZ4YlpCVVJL?=
 =?utf-8?B?aGRIb01GdUxZUGh4Vkd0TGttSnNFUTZpRmRwTVAwSzVRN2FMeWwrMFppQyto?=
 =?utf-8?B?OTJZSXdiTEFqSllJamVpb3UyZkEwZ3RJalY0WmhVampTZS9TYkdjeWVLQUdl?=
 =?utf-8?B?Uk94bFIwSzNoRzErRUUyY2NkVFA1bFBwd0dPY0toaTVZVFVBRytpMU44OWRE?=
 =?utf-8?B?SHdNZVQxa1RGaGxBbkVjZlRlV3ZtNERSNXBtaEx0ZmlhY21VS0xMZFBUQ3VS?=
 =?utf-8?B?c1gzeTZyV2ZGQWRuYU1XdWEwL1NlenFMOUhPbWtobDlNcGkxYlFKVmpkL2RV?=
 =?utf-8?B?RjBIMHdPTmNIcDBHaXdZTTMyS1RnOHVXWWdVOWlXRGtYWU91MHlJWW52YWJ2?=
 =?utf-8?B?ZWg4YzRQbUZzL0RvSHQvNThid2pOakZJRVFiVEt4cDRwZlE0Ylh0aFNLaDJH?=
 =?utf-8?B?MWtKT1M0UlRKa3dBWXZVbzJiR1JRS1FtdEdxR2pNbGc0SW5kb2pzNll2aU9I?=
 =?utf-8?B?angrNXY3TnlFSzFaaWo0ckpQc1ppcmFNRkJVc1JhSjJBTWZ0TEowS0NLTndV?=
 =?utf-8?B?Wk1NSmdjQnFUcG84cEtreGJpTG11OGpDMTZ2bzkyeEp6MUNTbDBvcmw2bDAz?=
 =?utf-8?B?bS9kbTBZSDlaYStBektTN2FDNXlsaXhhRjQ5UzBaRldPZzc0T0VVbE51NHdx?=
 =?utf-8?B?WGZpNDhydjFUb0F3RUZ0MUs4c2ttd2MwTDdvM1FuWjFGUVZoQlQ0UGlVZldS?=
 =?utf-8?B?WDAyeTNYUEMyZWlyaDlKZThQQ0hxTnlrSGlxZGpERW5SRUdxZ3I0d3hka0NB?=
 =?utf-8?B?eFNzRDdZZWNxOHlpMkpmM2RBN2xDNGZpZTgzb0VNTkg1QnhONlkzejhiSHdt?=
 =?utf-8?B?dUZ6ZjlLUjhsTEdtNmdrQkhMYlhGbGJTTXJqbjJCMVZuaXJPQVR0NGNCQ0Jn?=
 =?utf-8?B?Y1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7b0ca415-2c9c-4ab8-f232-08dc6bc9275f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 23:31:27.4096
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SgDnaFq0/hyjeW89qZbHAZa0TmDZLkWWyFB4sH250CiSd2g53v6oA2PCzAuM19DMojXH2Hi/lDSb9DEDFSOCormGk6QYB/zK1P2Z1VsitT4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6618
X-OriginatorOrg: intel.com

Hi Babu,

On 3/28/2024 6:06 PM, Babu Moger wrote:
> AMD Hardware provides a set of counters when the ABMC feature is supported.
> These counters are used for assigning events to the resctrl group.
> 
> Introduce the bitmap assign_cntrs_free_map to allocate and free the
> counters.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> 
> ---
> 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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index f49073c86884..2c7583e7b541 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -186,6 +186,22 @@ bool closid_allocated(unsigned int closid)
>  	return !test_bit(closid, &closid_free_map);
>  }
>  
> +static u64 assign_cntrs_free_map;
> +static u32 assign_cntrs_free_map_len;

Please provide summary in comments about what these globals are and how they
are used.

> +
> +static void assign_cntrs_init(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +
> +	if (r->mbm_assign_cntrs > 64) {
> +		r->mbm_assign_cntrs = 64;
> +		WARN(1, "Cannot support more than 64 Assignable counters\n");

I am a bit confused here. The configuration registers are introduced in patch #10
and if I counted right there are 5 bits for the counter id. It thus seems to me
as though there needs to be some checking during enumeration time to ensure
that all counters enumerated can be configured.

> +	}
> +
> +	assign_cntrs_free_map = BIT_MASK(r->mbm_assign_cntrs) - 1;

Please use bitmap API. For example, bitmap_fill()

> +	assign_cntrs_free_map_len = r->mbm_assign_cntrs;
> +}
> +
>  /**
>   * rdtgroup_mode_by_closid - Return mode of resource group with closid
>   * @closid: closid if the resource group
> @@ -2459,6 +2475,9 @@ static int resctrl_abmc_setup(enum resctrl_res_level l, bool enable)
>  	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>  	struct rdt_domain *d;
>  
> +	/* Reset the counters bitmap */
> +	assign_cntrs_init();
> +

(At this point it is unclear when resctrl_abmc_setup() is called to understand
if reset of bitmap may be appropriate. Please do expand all changelogs to help
readers along with how this implementation is intended to work.)

>  	/* Update QOS_CFG MSR on all the CPUs in cpu_mask */
>  	list_for_each_entry(d, &r->domains, list) {
>  		on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);

Reinette