Received-SPF: pass (google.com: domain of linux-kernel+bounces-168417-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: <b5e68d85-4bf2-4e55-a9c1-b39cb7d94db6@intel.com>
Date: Fri, 3 May 2024 16:33:58 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 12/17] x86/resctrl: Add the functionality to assign
 the RMID
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>
 <a6cb26639976873b1b85ec1606c2168493120ba3.1711674410.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <a6cb26639976873b1b85ec1606c2168493120ba3.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?OHgzZmoxSWJpWHFXODhMeHlwYkI3TEJRcHNPZDhkTXc4bHZXcVEzRmlRQktN?=
 =?utf-8?B?R3AxL2VKVHhQTFlSTzhWS1dZVlVHNW5iaXo3VUQ4bWZuaVNnTWtYbUpGaHBQ?=
 =?utf-8?B?Mld3Q3F1ckhEcVdyOFBuRU9jc0JWa1FuQzltYzhJdXZHaTFadWwwckkxTWgv?=
 =?utf-8?B?eHVIYk9zS1hwYVRHWEkxRTNWSWU1YnF6cm9IWVYzeTRBTlRMYk9ZMlludTVY?=
 =?utf-8?B?R2gwWlUvQ0V6WEgwMFNtamtweVZFS3B4cHd5ZlR2Rm5vZlRRelJkS1BWWnFp?=
 =?utf-8?B?NzZlY2d1Zksra25XYkovYXRFK25YNDkrRkVpRFQ4bmVoYS9Xd1pTL1Q0WEQ5?=
 =?utf-8?B?bXVSbTBKYUNGL2ZGUzV6WUhhNlR3TEs1cjBZVHBWeDhXcCtYWkdrRzkzOTNJ?=
 =?utf-8?B?L005WFJQNkR1ZTlBTkNhRHFDK0diQWNVNW5UajVTWkZGait1RGtlTGRpUzN1?=
 =?utf-8?B?aVk4VDhsQloxenZlLys4bHhrNWI5RjVQc09pc2IrWnZvSUpHU1QxdGVmRmN5?=
 =?utf-8?B?aUlmTnNyREdTdWNWdjE1cGtVTjZPMXdaMEIzTkk3WC9TZUIxbzB3K0YzUm50?=
 =?utf-8?B?Myt5NGNXcU5pbjBWNVRiN1kwK1hTWjYzaWxzUVdOYXIzeC93WERHQU81OXNF?=
 =?utf-8?B?Q3VsTFJOd3R1L0tLUy8vQitwZkxWcFArYUhEZ2VHSytuRTZFWVkrSVliZTZt?=
 =?utf-8?B?eG9wbkliejUxK3hjNk1uWmpVSmRrdDBrV1ZGY1RRUGx0eW56a2x3eWhtTm1K?=
 =?utf-8?B?V1dsdE5MQzFTeVlIVCs5VTRtejR0cDZUMmM5SkwvZ0V5ekJLM2tzd0pKOUFs?=
 =?utf-8?B?eXdQZEJqU0x2TlJ3eDhsOUJqTmNjK1ZnNDJCMGQrdUhHS2xDV3FvTFVoZlkv?=
 =?utf-8?B?a2ZOTys5KzJPNmExR3RYOHlOTWE1d1lVOFgyMkJValRjMkM0U24rNFludEt2?=
 =?utf-8?B?OEMzUEoxQ2oyM1JEcytJVitVVk16N1gzUGRJZ3FTVVlSVEIxK3VCeStqLytI?=
 =?utf-8?B?T2xmS0RkT2JtMGg4QXF3TitHLzZ6c0xOWGU5dVFzMkRyL2EvVHcrSERTb0N4?=
 =?utf-8?B?YTlBdUtuWGZrc1Z1dERPdW5tdzg2eDVpZmNRcTVrenEreGczV2tXY2VqMGhB?=
 =?utf-8?B?Y2ZyVWxVYWRUdGxKYXZIQXdnRDNrYXFBTzR4dnp5QVY1cVJ4dTRUMVFaR0l4?=
 =?utf-8?B?elFKM3k4Y2wxK1FMaEhNY01peXFiTktRRXNjaXRGOHR1YlMzWjFRZWJ3algv?=
 =?utf-8?B?eFhNdjFHMkFYclYwN1F0UG5BQUg3WGY5aWZoQXN0Z2ZrSzNzVHc5ZHpkV000?=
 =?utf-8?B?OVBQNy9CMWVHM0gwV2VKS084V1ZyZEdzd1AyaUtzV0Z4MEZYVmxTOXFiR3ly?=
 =?utf-8?B?OWEwSkZiN2Q4Y2hCeDB0SjluUFNZdFhLZ2loK2VHZGpIVVJTYmhUblJPZ1RG?=
 =?utf-8?B?eVlJSGVmV2R0NllVbUo3b1d3dFpMMHdBamtpYUpmQXVOSHpkZjJPTTIzYkpM?=
 =?utf-8?B?QzZRM2JzWUd5dWwxYUdhTmhORitZZGRCZTRpa202bDNVZ21ST1FaelprbldR?=
 =?utf-8?B?SFlHbEtGcDJ5V0NWZHMzU29LUWJCODBNZlhZVnVhVzZCQURtTXFxOWRhd2FD?=
 =?utf-8?B?Qk0xYmZjUldWOHl3OTRhV2V4WHBlaEhwam82MW1oc29BUk1jVExmRHNBR00v?=
 =?utf-8?B?ZThGR2xTeU94cHl6THlpcW9JdnY4K3VSUWw3QTVpWFBmQmhxSDNzbCtCek8x?=
 =?utf-8?B?dVROUVd1UkNQQVgrdjlkc3ozb3dmQkRDM0I3QVhoZDc2d3BNSGwxWExuOUpC?=
 =?utf-8?B?ZUVNYkxZaUl5TkNnc0swaFRFR2U0QU1NRzdabTlNUnlNeUVXNlVkUGFHK3hQ?=
 =?utf-8?B?YjRQQnVYdmhLRUdXaTdIdDRjeFk0LzdqSktreXMyZWxrSDBRdkNzRU1nbC9Q?=
 =?utf-8?B?bXY5dzNXblJSbHZWSElxb0JrZDQyR0hBeDU1U1k2QmU5Q3dUV0Jic2NFZTRj?=
 =?utf-8?B?WDBBaWN6eWZ0RjNQMDlDSUVMOXhmaUlXN2kydTJUaVVwMVlOSHh6cGZ6di9m?=
 =?utf-8?B?V1FDY2RqcEgzdHpMUUxJb29DSWVQdlkvUFVtbW4rR0lkU0hEVDdWanNuWjFG?=
 =?utf-8?B?KzlJTWVNWXVLUGQ2aVU5QzNTMjZzNzdFMVhOSGgwZllTWUNLbHpkUSsyZFky?=
 =?utf-8?B?ZWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d9af1b68-39af-4a3a-7de8-08dc6bc9833e
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:34:01.5659
 (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: mzrcVCpyRjVnieqgY7LhzfwWWpsH5mBA60RyR8oYPcJUFXCipxXCSV/xzWJcHRGdoWeBnzsHTjEPr2lXE6YRd6e3Fj2CuwNyJo/stcPf4hU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB8076
X-OriginatorOrg: intel.com

Hi Babu,

On 3/28/2024 6:06 PM, Babu Moger wrote:
> With the support of ABMC (Assignable Bandwidth Monitoring Counters)
> feature, the user has the option to assign or unassign the RMID to
> hardware counter and monitor the bandwidth for the longer duration.

What is meant with "the longer duration" (this term is used throughout
this series)? Perhaps "for as long as a hardware counter is assigned"?

> 
> Provide the interface to assign the counter to the group.
> 
> The ABMC feature implements a pair of MSRs, L3_QOS_ABMC_CFG (MSR
> C000_03FDh) and L3_QOS_ABMC_DSC (MSR C000_3FEh). Each logical processor
> implements a separate copy of these registers. Attempts to read or write
> these MSRs when ABMC is not enabled will result in a #GP(0) exception.
> 
> Individual assignable bandwidth counters are configured by writing to
> L3_QOS_ABMC_CFG MSR and specifying the Counter ID, Bandwidth Source, and
> Bandwidth Types. Reading L3_QOS_ABMC_DSC returns the configuration of the
> counter specified by L3_QOS_ABMC_CFG [CtrID].

This mentions the AMD architecture parts needing configuration but not what
resctrl parts are used to accomplish this configuration. It is difficult to
understand this work without this connection.

> 
> 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
> ---
> v3: Removed the static from the prototype of rdtgroup_assign_abmc.
>     The function is not called directly from user anymore. These
>     changes are related to global assignment interface.
> 
> v2: Minor text changes in commit message.
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 ++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 88453c86474b..9d84c80104f9 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -651,6 +651,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  void __init resctrl_file_fflags_init(const char *config,
>  				     unsigned long fflags);
>  void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
> +ssize_t rdtgroup_assign_abmc(struct rdtgroup *rdtgrp, u32 evtid, int mon_state);
>  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 7f54788a58de..cfbdaf8b5f83 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -202,6 +202,18 @@ static void assign_cntrs_init(void)
>  	assign_cntrs_free_map_len = r->mbm_assign_cntrs;
>  }
>  
> +static int assign_cntrs_alloc(void)
> +{
> +	u32 counterid = ffs(assign_cntrs_free_map);
> +
> +	if (counterid == 0)
> +		return -ENOSPC;
> +	counterid--;
> +	assign_cntrs_free_map &= ~(1 << counterid);
> +
> +	return counterid;

Use bitmap API ... eg. find_first_bit() (eliminates
need to adjust counterid), __clear_bit()

> +}
> +
>  /**
>   * rdtgroup_mode_by_closid - Return mode of resource group with closid
>   * @closid: closid if the resource group
> @@ -1848,6 +1860,80 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>  	return ret ?: nbytes;
>  }
>  
> +static void rdtgroup_abmc_msrwrite(void *info)
> +{
> +	u64 *msrval = info;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *msrval);
> +}
> +
> +static void rdtgroup_abmc_domain(struct rdt_domain *d, struct rdtgroup *rdtgrp,
> +				 u32 evtid, int index, bool assign)
> +{
> +	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
> +	struct arch_mbm_state *arch_mbm;
> +
> +	abmc_cfg.split.cfg_en = 1;
> +	abmc_cfg.split.ctr_en = assign ? 1 : 0;
> +	abmc_cfg.split.ctr_id = rdtgrp->mon.abmc_ctr_id[index];
> +	abmc_cfg.split.bw_src = rdtgrp->mon.rmid;
> +
> +	/*
> +	 * Read the event configuration from the domain and pass it as
> +	 * bw_type.
> +	 */
> +	if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
> +		abmc_cfg.split.bw_type = hw_dom->mbm_total_cfg;
> +		arch_mbm = &hw_dom->arch_mbm_total[rdtgrp->mon.rmid];
> +	} else {
> +		abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg;
> +		arch_mbm = &hw_dom->arch_mbm_local[rdtgrp->mon.rmid];
> +	}
> +
> +	smp_call_function_any(&d->cpu_mask, rdtgroup_abmc_msrwrite, &abmc_cfg, 1);
> +
> +	/* Reset the internal counters */
> +	if (arch_mbm)
> +		memset(arch_mbm, 0, sizeof(struct arch_mbm_state));
> +}
> +
> +ssize_t rdtgroup_assign_abmc(struct rdtgroup *rdtgrp, u32 evtid, int mon_state)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	int counterid = 0, index;
> +	struct rdt_domain *d;
> +
> +	if (rdtgrp->mon.mon_state & mon_state) {
> +		rdt_last_cmd_puts("ABMC counter is assigned already\n");
> +		return 0;
> +	}
> +
> +	index = mon_event_config_index_get(evtid);
> +	if (index == INVALID_CONFIG_INDEX) {
> +		pr_warn_once("Invalid event id %d\n", evtid);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Allocate a new counter and update domains
> +	 */
> +	counterid = assign_cntrs_alloc();
> +	if (counterid < 0) {
> +		rdt_last_cmd_puts("Out of ABMC counters\n");
> +		return -ENOSPC;
> +	}
> +
> +	rdtgrp->mon.abmc_ctr_id[index] = counterid;
> +
> +	list_for_each_entry(d, &r->domains, list)
> +		rdtgroup_abmc_domain(d, rdtgrp, evtid, index, 1);
> +
> +	rdtgrp->mon.mon_state |= mon_state;
> +
> +	return 0;
> +}
> +
>  /* rdtgroup information files for one cache resource. */
>  static struct rftype res_common_files[] = {
>  	{

It is not clear to me where the filesystem and architecture boundaries
are, but I understand that you and Peter already discussed this and I look
forward to next version that will make this easier to understand.

Reinette