Date: Thu, 13 Jun 2024 18:48:28 -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 14/19] x86/resctrl: Add the interface to assign ABMC
 counter

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> ABMC feature requires to users to assign a hardware counter to an RMID

"requires to users" -> "requires users"?

> to monitor the events. Provide the interfaces to assign a counter.
> 
> Individual counters are configured by writing to L3_QOS_ABMC_CFG MSR
> and specifying the counter id, bandwidth source, and bandwidth types.

Where is discription of what this patch does and why?

This and following patches seem to introduce building blocks that
are only used later in series. These building blocks are introduced
without any information about what they do and why and that makes it
difficult to understand how this work will fall into place.

> 
> 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@....com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
> v4: Commit message update.
>      User bitmap APIs where applicable.
>      Changed the interfaces considering MPAM(arm).
>      Added domain specific assignment.
> 
> 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 |   3 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 101 +++++++++++++++++++++++++
>   2 files changed, 104 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 45ed33f4f0ff..a88c8fc5e4df 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -657,6 +657,9 @@ 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);
> +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);
>   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 0e425c91fa46..48df76499a04 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -203,6 +203,19 @@ static void num_cntrs_init(void)
>   	num_cntrs_free_map_len = r->mon.num_cntrs;
>   }
>   
> +static int assign_cntrs_alloc(void)

Only one counter is allocated and "assign" is unexpected since it
seems to imply how this counter will be used.
It can just be "mon_cntr_alloc()"?

> +{
> +	u32 ctr_id = find_first_bit(&num_cntrs_free_map,
> +				    num_cntrs_free_map_len);
> +
> +	if (ctr_id >= num_cntrs_free_map_len)
> +		return -ENOSPC;
> +
> +	__clear_bit(ctr_id, &num_cntrs_free_map);
> +
> +	return ctr_id;
> +}
> +
>   /**
>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>    * @closid: closid if the resource group
> @@ -1830,6 +1843,94 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>   	return ret ?: nbytes;
>   }
>   
> +static void rdtgroup_abmc_cfg(void *info)
> +{
> +	u64 *msrval = info;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *msrval);
> +}
> +

Please add comments to these functions to summarize what
they do.

> +int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
> +			u32 ctr_id, u32 closid, bool enable)
> +{
> +	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 = enable ? 1 : 0;
> +	abmc_cfg.split.ctr_id = ctr_id;
> +	abmc_cfg.split.bw_src = 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[rmid];
> +	} else {
> +		abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg;
> +		arch_mbm = &hw_dom->arch_mbm_local[rmid];
> +	}
> +
> +	smp_call_function_any(&d->cpu_mask, rdtgroup_abmc_cfg, &abmc_cfg, 1);
> +
> +	/* Reset the internal counters */

"internal counters"? This needs a definition ... but since this is not
a new data structure the comment can be more specific about what is done
and why.

> +	if (arch_mbm)
> +		memset(arch_mbm, 0, sizeof(struct arch_mbm_state));
> +
> +	return 0;

If this function always succeeds then it can just be void?

> +}
> +
> +int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid)

Please use consistent naming. Note how the filename is "rdtgroup.c" and this
function operates on "struct rdtgroup" and if you take a closer look at
the functions in this file and the header where you add this function you
will notice a distinct pattern of "rdtgroup_" used as prefix. There really
is no reason to start a new namespace for this.

> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	int ctr_id = 0, index;
> +	struct rdt_domain *d;
> +	u32 mon_state;
> +
> +	index = mon_event_config_index_get(evtid);
> +	if (index == INVALID_CONFIG_INDEX) {
> +		rdt_last_cmd_puts("Invalid event id\n");
> +		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;
> +	}
> +
> +	/* Nothing to do if event has been assigned already */
> +	if (rdtgrp->mon.mon_state & mon_state) {

Why is this mon_state needed? Is it not redundant when considering that
struct mongroup has an array of counter IDs that can already be used
to see if a counter is assigned or not?
What if there is a new:
#define MON_CNTR_UNSET U32_MAX

When cntr_id[index] == MON_CNTR_UNSET then it is not assigned.

> +		rdt_last_cmd_puts("ABMC counter is assigned already\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * Allocate a new counter and update domains
> +	 */
> +	ctr_id = assign_cntrs_alloc();
> +	if (ctr_id < 0) {
> +		rdt_last_cmd_puts("Out of ABMC counters\n");
> +		return -ENOSPC;
> +	}
> +
> +	rdtgrp->mon.ctr_id[index] = ctr_id;
> +
> +	list_for_each_entry(d, &r->domains, list)
> +		resctrl_arch_assign(d, evtid, rdtgrp->mon.rmid, ctr_id,
> +				    rdtgrp->closid, 1);
> +
> +	rdtgrp->mon.mon_state |= mon_state;
> +
> +	return 0;
> +}
> +
>   /* rdtgroup information files for one cache resource. */
>   static struct rftype res_common_files[] = {
>   	{

Reinette

Powered by blists - more mailing lists