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@amd.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
>