Re: [PATCH v14 17/32] x86/resctrl: Implement resctrl_arch_config_cntr() to assign a counter with ABMC

[Moger, Babu]

Hi Reinette,

On 6/24/25 22:03, Reinette Chatre wrote:
> Hi Babu,
> 
> With the new arch API this have "x86,fs/resctrl" prefix.

Sure.

> 
> On 6/13/25 2:05 PM, Babu Moger wrote:
>> The ABMC feature allows users to assign a hardware counter ID to an RMID,
>> event pair and monitor bandwidth usage as long as it is assigned. The
>> hardware continues to track the assigned counter until it is explicitly
>> unassigned by the user.
>>
>> 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.
>>
>> 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).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
> 
> ...
> 
>> ---
>>  arch/x86/kernel/cpu/resctrl/monitor.c | 38 +++++++++++++++++++++++++++
>>  include/linux/resctrl.h               | 19 ++++++++++++++
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 0ad9c731c13e..6b0ea4b17c7a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -444,3 +444,41 @@ 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, 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;
>> +	if (assign)
>> +		abmc_cfg.split.bw_type = resctrl_get_mon_evt_cfg(evtid);
>> +
>> +	smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1);
> 
> An optimization to consider is to direct the IPI to a housekeeping CPU.
> If one exist, a further optimization could be to queue it on that CPU
> instead of IPI. Your call since I am not familiar with the use cases here.
> Looks like all paths leading to this is triggered by a user space action
> (mount, mkdir, or write to update event config). This would require exposing
> the housekeeping CPU code to arch.

Do you mean something like this?

cpu = cpumask_any_housekeeping(&d->hdr.cpu_mask, RESCTRL_PICK_ANY_CPU);

smp_call_on_cpu(cpu, resctrl_abmc_config_one_amd, &abmc_cfg, false);


You want to do these changes now or later?  It requires few other changes
to move around the code.

> 
>> +
>> +	/*
>> +	 * 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));
>> +	}
> 
> I am not able to recognize how the struct rdt_resource parameter is used. What am I missing?

No. It is not used here. It is kept as other arch's can use it.  I think
James commented about it earlier.

> 
>> +}
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index a58dd40b7246..1539d1faa1a1 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -594,6 +594,25 @@ 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 with its new RMID
>> + *				and event details.
>> + * @r:			Resource structure.
>> + * @d:			The domain in which the counter ID is to be configured.
> 
> "The domain in which the counter should be configured." or "The domain in which counter
> with ID @cntr_id should be configured."?

Added

"The domain in which counter with ID @cntr_id should be configured."


> 
>> + * @evtid:		Monitoring event type (e.g., QOS_L3_MBM_TOTAL_EVENT_ID
>> + *			or QOS_L3_MBM_LOCAL_EVENT_ID).
>> + * @rmid:		RMID.
>> + * @closid:		CLOSID.
>> + * @cntr_id:		Counter ID to configure.
>> + * @assign:		True to assign the counter, false to unassign
>> + *			the counter.
> 
> The changelog and comments only mention counter "assignment" but this same call
> is used to update an existing assignment, which on ABMC are done the same.
> This may be ok for now but I think it will be helpful to amend the above to say
> something like:
> 
>  * @assign:		True to assign the counter or update an existing assignment, false to unassign
>  *			the counter.

Sure.

> 
>> + *
>> + * This can be called from any CPU.
>> + */
>> +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, bool assign);
>> +
>>  extern unsigned int resctrl_rmid_realloc_threshold;
>>  extern unsigned int resctrl_rmid_realloc_limit;
>>  
> 
> Reinette
> 

-- 
Thanks
Babu Moger