Message-ID: <f51e2bec-b530-4d24-a72c-a5e41c82e984@amd.com>
Date: Fri, 20 Dec 2024 15:38:55 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 tony.luck@...el.com, peternewman@...gle.com
Cc: fenghua.yu@...el.com, x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 xiongwei.song@...driver.com, pawan.kumar.gupta@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, jpoimboe@...nel.org, perry.yuan@....com,
 sandipan.das@....com, kai.huang@...el.com, xiaoyao.li@...el.com,
 seanjc@...gle.com, xin3.li@...el.com, andrew.cooper3@...rix.com,
 ebiggers@...gle.com, mario.limonciello@....com, james.morse@....com,
 tan.shaopeng@...itsu.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, maciej.wieczor-retman@...el.com,
 eranian@...gle.com
Subject: Re: [PATCH v10 17/24] x86/resctrl: Add the interface to unassign a
 counter

Hi Reinette,

On 12/19/2024 5:32 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/12/24 12:15 PM, Babu Moger wrote:
>> The mbm_cntr_assign mode provides a limited number of hardware counters
>> that can be assigned to an RMID, event pair to monitor bandwidth while
>> assigned. If all counters are in use, the kernel will show an error
>> message: "Out of MBM assignable counters" when a new assignment is
>> requested. To make space for a new assignment, users must unassign an
>> already assigned counter.
>>
>> Introduce an interface that allows for the unassignment of counter IDs
>> from the domain.
> 
> Subject and changelog claims this introduces an interface, there is no new
> resctrl interface introduced here. Can this be more specific?

Sure. Let me rewrite the subject and description.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 52 ++++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 70d2577fc377..f858098dbe4b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -706,6 +706,8 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>   			     u32 cntr_id, bool assign);
>>   int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>>   			       struct rdt_mon_domain *d, enum resctrl_event_id evtid);
>> +int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +				 struct rdt_mon_domain *d, enum resctrl_event_id evtid);
> 
> (please use consistent parameter ordering)

Sure.

> 
>>   struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>>   				u32 rmid, enum resctrl_event_id evtid);
>>   #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 1c8694a68cf4..a71a8389b649 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1990,6 +1990,20 @@ static void mbm_cntr_free(struct rdt_resource *r, struct rdt_mon_domain *d,
>>   	}
>>   }
>>   
>> +static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id;
>> +
>> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +		if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
>> +		    d->cntr_cfg[cntr_id].evtid == evtid)
>> +			return cntr_id;
>> +	}
>> +
>> +	return -EINVAL;
> 
> This could be -ENOENT?

Sure.

> 
>> +}
> 
> mbm_cntr_get() seems to be essentially a duplicate of mbm_cntr_assigned() that returns
> actual counter ID instrad of true/false. Could only one be used?

Yes. We can use mbm_cntr_get() alone.

> 
>> +
>>   /*
>>    * Assign a hardware counter to event @evtid of group @rdtgrp.
>>    * Counter will be assigned to all the domains if rdt_mon_domain is NULL
>> @@ -2037,6 +2051,44 @@ int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>>   	return ret;
>>   }
>>   
>> +/*
>> + * Unassign a hardware counter associated with @evtid from the domain and
>> + * the group. Unassign the counters from all the domains if rdt_mon_domain
>> + * is NULL else unassign from the specific domain.
> 
> (same comment as previous patch about consistency in referring to function
> parameters)
> 
Sure.

>> + */
>> +int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +				 struct rdt_mon_domain *d, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id, ret = 0;
>> +
>> +	if (!d) {
>> +		list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +			if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
>> +				continue;
>> +
>> +			cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> +
> 
> It seems unnecessary to loop over array twice here. mbm_cntr_assigned() seems
> unnecessary. Return value of mbm_cntr_get() can be used to determine if it
> is assigned or not?

Yes. Sure.

> 
>> +			ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +						  rdtgrp->closid, cntr_id, false);
>> +			if (!ret)
>> +				mbm_cntr_free(r, d, rdtgrp, evtid);
> 
> ... and by providing cntr_id to mbm_cntr_free() another unnecessary loop can be avoided.

Sure.


> 
>> +		}
>> +	} else {
>> +		if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
>> +			goto out_done_unassign;
>> +
>> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> +
>> +		ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +					  rdtgrp->closid, cntr_id, false);
>> +		if (!ret)
>> +			mbm_cntr_free(r, d, rdtgrp, evtid);
>> +	}
>> +
>> +out_done_unassign:
>> +	return ret;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>   	{
> 
> Reinette
> 

Powered by blists - more mailing lists