Received-SPF: pass (google.com: domain of linux-kernel+bounces-214196-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <333339e1-5b94-4fac-8d18-f18c7780e6e1@intel.com>
Date: Thu, 13 Jun 2024 18:49:30 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 15/19] x86/resctrl: Add the interface to unassign ABMC
 counter
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.1716552602.git.babu.moger@amd.com>
 <1964d74c24b041e5c9589040701a6f6aaa839e19.1716552602.git.babu.moger@amd.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <1964d74c24b041e5c9589040701a6f6aaa839e19.1716552602.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?SVdjUm1Pc0w0TUErUlRyUkxYOGd0dGNIcTJBMnErMW5zcDRSdkFlUFp4bnVz?=
 =?utf-8?B?Z0hEZGx2ODd1SFdpVUYwMklxWmVLcEdFK1VWaDBNS3d3azRKRVUrZWpUQzZj?=
 =?utf-8?B?SUxDY2pSTURHNTEzT2JsWThxc0xmbFhOYTVyOEhDZFZZRWJXMS9FYTduMGtK?=
 =?utf-8?B?cVZFYmhvS0d5TjdkU1JFSTVZTnZjZXNWejN6dkxYWTBWR01yUjZlTmtCRUhn?=
 =?utf-8?B?Y3VKS1oxTnZ4QXpYZEFnQzdjRmdyeXJDSDN5YnZSV3J0RmF4bnFTNDI1L29u?=
 =?utf-8?B?aUlFb2V3VFE0eTV2OGxrbUZNc0NaVWR3Ly9uUUNCL28vZERtUkJEVndwN2l5?=
 =?utf-8?B?aHF6QlFxckt5TWZsZFNuRFhxcXVmMDY5NGh1TmZpOVJBNUc2cUw0bE5BSm5J?=
 =?utf-8?B?bU9tUWUrbEVQOURwbDZBWjlSVVh4dnVZMnJITW5laktWT0RQRlloeEFRcWgr?=
 =?utf-8?B?Wk9UeUwyUVNJQ1BXZXgvaG1GM3ZGT05jM0JsSjhzQmVtZ2xER29lVi9uY2pG?=
 =?utf-8?B?SXcvbkVWV282VFpUUHNYNm5rejlvVHpSRlRmL0RERjhDT0U5UytkVzBtRnhT?=
 =?utf-8?B?VlRWcE5aS3FMMkpTYVlMZE92MG9uTGxOTDJySGNOWXpVWTZsMUVLZFNoYXBJ?=
 =?utf-8?B?VzkwK3FBTnh4Yk0rU0tpalpSTEhmMjlOMnp5RTBSY1hJbHh2N242azg2eWk5?=
 =?utf-8?B?cUlYM3NXQm5zUnllckRES0RkU0loeUZsTGNzWkRicS9qbW0vVkdRbEFmb0pI?=
 =?utf-8?B?ZGxNSFNONWhDdzM2bnBjdUpGNlBFZm5NelhuR0VwRGxLaGNvTHQ2YTB1M2xJ?=
 =?utf-8?B?MzkySzJVL0ZMWUVKUnVJSVR4U1BMSEpJNWpib1ljUW81dEpaV2dibkNjazVu?=
 =?utf-8?B?NUhLREZJYmhsL3pyek82MWhxRnlpbmJmN3dYVEhRM0ZPS3JZenFjc0tzS0ZM?=
 =?utf-8?B?cVlyYlkyNlV5am5BNEZtc0piNG1LMnhjUENRSkxGYnJIdElDN0lhY3R2UXlL?=
 =?utf-8?B?UllDb0ZxaUpzRXQyQnkwR0Q1RlFLZDFvd1E3Rk4xTHYxUzBhSEJ1eG5GbUhO?=
 =?utf-8?B?Q3J0ZG1vN3lkVHEzdXFvWVF4RFhWaEF3T01yTlRjNXJVK3AvR0l0NU9pT1Rj?=
 =?utf-8?B?NGluNWVBSXJucUZsaDRBODhzNWIva20wUUVIb2poMnhRbjJVZ3AvWjRtejRD?=
 =?utf-8?B?cXlkMXZPVUF6QXpNZFdMMFhONEw5ZkRMZkdpdUZHem9naDhOeXVlMjZyNGlT?=
 =?utf-8?B?MUNaQnJIaThYdFJzR1pXUHI3WVhoZzIwc2FrRDg4b0pYOGxPQVdhanlkdFJT?=
 =?utf-8?B?a1ZJVDZOZ0ZnZVRCclp1U1VXVDlFS01FTWViMmhHc0ZDYm9CZ016Q3ZndFNC?=
 =?utf-8?B?b3l6Z0hnOHYrY3kra2Y5VU5xOTZVbTM5dWZndkFTZElmVzhFa29XRmNad1NM?=
 =?utf-8?B?TE12RmtKdzhEcFk3RGd5ZFVRQTJ3NytZelVPd3c0SFNtVThCbFU0NFdQMlVn?=
 =?utf-8?B?dUZueHRXRUYwQWRHaTExWm84ZTR2Q0tWYnF4cThsRXFFSVE1NmtGTnpvU0Rm?=
 =?utf-8?B?VWM3LzZLQjhkRUluVHJ4UkppWVZrMUZ2MDJmL1dsN1NDQXBKVTVxQmpZRmw4?=
 =?utf-8?B?NFpVMk1vc0RkVGMyeXRwaVE2akd6c2toRWtFTWJ1bzJkbnRiOWRnbXdDWFJm?=
 =?utf-8?B?NG9WV3l0VHZyL1dsaEl1eHpvY09NZ2c3dmRBM0ZieG4rdnV4bXZMN2VKNnpB?=
 =?utf-8?B?ekFRSGVxdFNKMDdXYjFDZXROWjRhYVBGcUQvaG90Ky9DZU9IWElNLzBFTnZ6?=
 =?utf-8?B?RGFDc3FhUEhtUU53azRzTnRSVVlpVzJMYlJTa0ZWcVJvUWIrYi84TnNjTDdp?=
 =?utf-8?B?OU9YSWRyRnhIaG1rRzdzNnBveTdkdnJIYXVTUkt6NkRTc3c5SEJCdXpDZ1oy?=
 =?utf-8?B?NDh2SXFMQzh4WVgxSlZIQlYrNkJkOGxHWVdQTHFadmppV2ZMWlZxcVQ0eE81?=
 =?utf-8?B?MmpEWTM3UlpkNnVIeXd6U0pCTGVrSmZnMVZLM2pXZEpMRnl1Y1lubkdvUWxk?=
 =?utf-8?B?SkFUS2IwOHJzV0lEUEdnYXNyQXdNOXhpVUZVam00aFlZS21MamZlNHg2UHdR?=
 =?utf-8?B?anZJWm9ia3N6K0FvUG44dDBhMWx1cFNiYXRZcExUaDM0b1VrNWNQa09vR0hk?=
 =?utf-8?B?emc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d2c9a09-3714-474c-5c0a-08dc8c1440b4
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 01:49:39.3481
 (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: cV8ITQXDuQIr9B1hLst59f6Bef0Ix90v6hZ+vetHalxmOVXxqO4P6wq0f2WPKfH8YxvU5r1RofuKY/ESnAmcqDYCrmJdrINrSUKjxiWadSQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6203
X-OriginatorOrg: intel.com

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> Hardware provides a limited number of ABMC counters. Once all the
> counters are exhausted, counters need to be freed for new assignments.
> 
> Provide the interface to unassign the counter.

Please write a proper changelog. This needs information that explains
what this patch does and why.

> 
> The feature details are documented in the 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
> ---
> v4: Added domain specific unassign feature.
>      Few name changes.
> 
> v3: Removed the static from the prototype of rdtgroup_unassign_abmc.
>      The function is not called directly from user anymore. These
>      changes are related to global assignment interface.
> 
> v2: No changes.
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h |  2 ++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a88c8fc5e4df..e16244895350 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -660,6 +660,8 @@ void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
>   int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
>   			u32 ctr_id, u32 closid, bool enable);
>   int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid);
> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid);
> +void num_cntrs_free(u32 ctr_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/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 48df76499a04..5ea1e58c7201 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -216,6 +216,11 @@ static int assign_cntrs_alloc(void)
>   	return ctr_id;
>   }
>   
> +void num_cntrs_free(u32 ctr_id)

The name does not reflect what it does. It neither frees the "num_cntrs" information
nor does it free "num_cntrs" of counters. How about "mon_cntr_free()"?


> +{
> +	__set_bit(ctr_id, &num_cntrs_free_map);
> +}
> +
>   /**
>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>    * @closid: closid if the resource group
> @@ -1931,6 +1936,43 @@ int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid)
>   	return 0;
>   }
>   
> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid)

Same comment wrt namespace. Also this function needs a description.

> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_domain *d;
> +	u32 mon_state;
> +	int index;
> +
> +	index = mon_event_config_index_get(evtid);
> +	if (index == INVALID_CONFIG_INDEX) {
> +		pr_warn_once("Invalid event id %d\n", evtid);
> +		return -EINVAL;
> +	}
> +
> +	if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
> +		mon_state = ASSIGN_TOTAL;
> +	} else if (evtid == QOS_L3_MBM_LOCAL_EVENT_ID) {
> +		mon_state = ASSIGN_LOCAL;
> +	} else {
> +		rdt_last_cmd_puts("Invalid event id\n");
> +		return -EINVAL;
> +	}
> +
> +	if (rdtgrp->mon.mon_state & mon_state) {
> +		list_for_each_entry(d, &r->domains, list)
> +			resctrl_arch_assign(d, evtid, rdtgrp->mon.rmid,
> +					    rdtgrp->mon.ctr_id[index],
> +					    rdtgrp->closid, 0);
> +
> +		/* Update the counter bitmap */
> +		num_cntrs_free(rdtgrp->mon.ctr_id[index]);
> +	}
> +
> +	rdtgrp->mon.mon_state &= ~mon_state;
> +
> +	return 0;
> +}
> +
>   /* rdtgroup information files for one cache resource. */
>   static struct rftype res_common_files[] = {
>   	{


Reinette