Message-ID: <08f37011-baa1-40c6-815f-1ed89d19c90d@amd.com>
Date: Wed, 28 May 2025 16:39:57 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tony.luck@...el.com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com
Cc: james.morse@....com, dave.martin@....com, fenghuay@...dia.com,
 x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 ardb@...nel.org, gregkh@...uxfoundation.org, daniel.sneddon@...ux.intel.com,
 jpoimboe@...nel.org, alexandre.chartre@...cle.com,
 pawan.kumar.gupta@...ux.intel.com, thomas.lendacky@....com,
 perry.yuan@....com, seanjc@...gle.com, kai.huang@...el.com,
 xiaoyao.li@...el.com, kan.liang@...ux.intel.com, xin3.li@...el.com,
 ebiggers@...gle.com, xin@...or.com, sohil.mehta@...el.com,
 andrew.cooper3@...rix.com, mario.limonciello@....com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 peternewman@...gle.com, maciej.wieczor-retman@...el.com, eranian@...gle.com,
 Xiaojian.Du@....com, gautham.shenoy@....com
Subject: Re: [PATCH v13 11/27] x86/resctrl: Implement
 resctrl_arch_config_cntr() to assign a counter with ABMC

Hi Reinette,

On 5/22/2025 4:51 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/15/25 3:51 PM, Babu Moger wrote:
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it
>> is assigned. The assigned RMID will be tracked by the hardware until the
>> user unassigns it manually.
> 
> (please review this often repeated snippet to match new design)

Sure.
> 
>>
>> Implement an architecture-specific handler to assign and unassign the
>> counter. Configure counters by writing to the L3_QOS_ABMC_CFG MSR,
>> specifying the counter ID, bandwidth source (RMID), and event
>> configuration.
>>
> 
> ...
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index ff4b2abfa044..e31084f7babd 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -448,3 +448,40 @@ inline bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r)
>>   {
>>   	return resctrl_to_arch_res(r)->mbm_cntr_assign_enabled;
>>   }
>> +
>> +static void resctrl_abmc_config_one_amd(void *info)
>> +{
>> +	union l3_qos_abmc_cfg *abmc_cfg = info;
>> +
>> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg->full);
>> +}
>> +
>> +/*
>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>> + */
>> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> +			      u32 cntr_id, u32 evt_cfg, bool assign)
>> +{
>> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +	struct arch_mbm_state *am;
>> +
>> +	abmc_cfg.split.cfg_en = 1;
>> +	abmc_cfg.split.cntr_en = assign ? 1 : 0;
>> +	abmc_cfg.split.cntr_id = cntr_id;
>> +	abmc_cfg.split.bw_src = rmid;
>> +	abmc_cfg.split.bw_type = evt_cfg;
> 
> Is evt_cfg really needed to be programmed when unassigning a counter? Looking ahead at
> patch #14 resctrl_free_config_cntr() needs to go through extra list walk to get this data
> but why would hardware need an accurate event configuration to *unassign* a counter?

evt_cfg is not required during unassign. I can remove it.

> 
> It seems unnecessary to provide both the event ID *and* the configuration.
> resctrl_arch_config_cntr() could drop the "evt_cfg" parameter and instead there
> can be a new resctrl utility that architecture can use to query the event's configuration.
> Similar to resctrl_is_mon_event_enabled() introduced in
> https://lore.kernel.org/lkml/20250521225049.132551-3-tony.luck@intel.com/ that exposes an
> event property.

Sounds good.
I can add a new function resctrl_get_mon_event_config(evtid) and call it 
only during the "assign". It will be called inside 
resctrl_arch_config_cntr().

> 
> It looks to me as though there are a couple of changes in the telemetry work
> that would benefit this work. https://lore.kernel.org/lkml/20250521225049.132551-2-tony.luck@intel.com/
> switches the monitor events to be maintained in an array indexed by event ID, eliminating the
> need for searching the evt_list that this work does in a couple of places. Also note the handy
> new for_each_mbm_event() helper (https://lore.kernel.org/lkml/20250521225049.132551-5-tony.luck@intel.com/).

Sure. Looking at it now.

> 
> 
>> +
>> +	smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1);
>> +
>> +	/*
>> +	 * The hardware counter is reset (because cfg_en == 1) so there is no
>> +	 * need to record initial non-zero counts.
>> +	 */
>> +	if (assign) {
>> +		am = get_arch_mbm_state(hw_dom, rmid, evtid);
>> +		if (am)
>> +			memset(am, 0, sizeof(*am));
>> +	}
>> +}
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index d77981d1fcb9..59a4fe60ab46 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -559,6 +559,23 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *
>>    */
>>   void resctrl_arch_reset_all_ctrls(struct rdt_resource *r);
>>   
>> +/**
>> + * resctrl_arch_config_cntr() - Configure the counter id to RMID, event
>> + *				pair on the domain.
> 
> The sentence seem strange, should "Configure the counter" perhaps be
> "Assign the counter"? Or if the naming requires "configure" ...
> "Configure the counter with its new RMID and event details."? Please feel
> free to improve.

Last one looks good.

> 
>> + * @r:			Resource structure.
>> + * @d:			Domain that the counter id to be configured.
> 
> I am unable to parse description of @d.

The domain in which the counter ID is to be configured.

> 
>> + * @evtid:		Event type to configure.
>> + * @rmid:		RMID to configure.
>> + * @closid:		CLOSID to configure.
>> + * @cntr_id:		Counter ID to configure.
> 
> All four parameters descriptions end with "to configure" ... but it is actually only
> the counter that is configured while the rest is the data that the counter is configured with, no?

Will remove "to configure" from all the other fields except the cntr_id.

> 
>> + * @evt_cfg:		MBM event configuration value representing reads,
>> + *			writes etc.
> 
> Needs definition about what the contents of @evt_cfg means. This is the API ...it
> cannot be vague like "reads, write, etc." but should be specific about which bit means
> what.

Copying your comment on other patch
https://lore.kernel.org/lkml/14ca1527-ee25-448d-949b-ed8df546c916@intel.com/

@evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM, 
READS_TO_REMOTE_MEM, etc. bits that represent the memory transactions 
being counted.


> 
>> + * @assign:		Assign or unassign.
> 
> "True to assign the counter, false to unassign the counter."
> 

Sure.

> 
> Needs some context here about what architecture can expect on how this function will
> be called. For example, "Can be called from any CPU."
> 

Sure.

>> + */
>> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> +			      u32 cntr_id, u32 evt_cfg, bool assign);
>> +
>>   extern unsigned int resctrl_rmid_realloc_threshold;
>>   extern unsigned int resctrl_rmid_realloc_limit;
>>   
> 
> Reinette
> 

Thanks
Babu

Powered by blists - more mailing lists