[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3172b5fe-5394-92ca-4d4a-d194b84e8364@amd.com>
Date: Thu, 17 Oct 2024 18:11:08 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
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 v8 18/25] x86/resctrl: Add the interface to unassign a MBM
counter
Hi Reinette,
On 10/15/2024 10:29 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/9/24 10:39 AM, 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 both the group and the domain. Additionally, ensure that the global
>> counter is released if it is no longer assigned to any domains.
>
> Needs imperative tone ... "Release the global counter ..."
sure.
>
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
> ...
>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 2 +
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 56 ++++++++++++++++++++++++++
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 900e18aea2c4..6f388d20fb22 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -717,6 +717,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);
>> 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 e4f628e6fe65..791258adcbda 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1945,6 +1945,62 @@ int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> return ret;
>> }
>>
>> +static bool mbm_cntr_assigned_to_domain(struct rdt_resource *r, u32 cntr_id)
>> +{
>> + struct rdt_mon_domain *d;
>> +
>> + list_for_each_entry(d, &r->mon_domains, hdr.list)
>> + if (test_bit(cntr_id, d->mbm_cntr_map))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Unassign a hardware counter from the domain and the group.
>
> Not sure ... maybe "Unassign a hardware counter associated with @evtid from
> the domain and the group."?
ok.
>
>> + * Counter will be unassigned in all the domains if rdt_mon_domain is NULL
>
> Please use imperative tone: "Unassign the counter from all the domains ...."
ok.
>
>> + * else the counter will be assigned to specific domain.
>
> copy&paste error?
> "assigned to specific domain" -> "unassign from specific domain"?
ok.
>
>> + * Global counter will be freed once it is unassigned from all the domains.
>
> Needs imperative tone.
>
>> + */
>> +int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> + struct rdt_mon_domain *d, enum resctrl_event_id evtid)
>> +{
>> + int index = MBM_EVENT_ARRAY_INDEX(evtid);
>> + int cntr_id = rdtgrp->mon.cntr_id[index];
>> + int ret;
>> +
>> + /* Return early if the counter is unassigned already */
>> + if (cntr_id == MON_CNTR_UNSET)
>> + return 0;
>> +
>> + if (!d) {
>> + list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> + ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> + rdtgrp->closid, cntr_id, false);
>> + if (ret)
>> + goto out_done_unassign;
>> +
>> + clear_bit(cntr_id, d->mbm_cntr_map);
>> + }
>> + } else {
>> + ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> + rdtgrp->closid, cntr_id, false);
>> + if (ret)
>> + goto out_done_unassign;
>> +
>> + clear_bit(cntr_id, d->mbm_cntr_map);
>
> Please see comment to previous patch about the duplicate snippets. Snippets can be
> replaced with single function that also resets architectural state.
Sure.
will combine rdtgroup_assign_cntr_event() and
rdtgroup_unassign_cntr_event().
I need to rename the function. How about resctrl_configure_cntr_event()?
>
>> + }
>> +
>> + /* Update the counter bitmap */
>
> What is the update?
Clear the counter bitmap.
>
>> + if (!mbm_cntr_assigned_to_domain(r, cntr_id)) {
>> + mbm_cntr_free(r, cntr_id);
>> + rdtgrp->mon.cntr_id[index] = MON_CNTR_UNSET;
>> + }
>> +
>> +out_done_unassign:
>> + return ret;
>> +}
>> +
>> /* rdtgroup information files for one cache resource. */
>> static struct rftype res_common_files[] = {
>> {
>
>
> Reinette
>
--
- Babu Moger
Powered by blists - more mailing lists