Hi Reinette, On 8/16/24 16:41, Reinette Chatre wrote: > Hi Babu, > > On 8/6/24 3:00 PM, Babu Moger wrote: >> The ABMC feature provides an option to the user to assign a hardware > > This is about resctrl fs so "The ABMC feature" -> "mbm_cntr_assign mode" Sure. > (please check whole series). Sure. > >> counter to an RMID 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. >> >> Hardware provides only limited number of counters. If the system runs out >> of assignable counters, kernel will display an error when a new assignment >> is requested. Users need to unassign a already assigned counter to make >> space for new assignment. >> >> Provide the interface to unassign the counter ids from the group. Free the >> counter if it is not assigned in any of the domains. >> >> 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> >> --- >> v6: Removed mbm_cntr_free from this patch. >> Added counter test in all the domains and free if it is not >> assigned to >> any domains. >> >> v5: Few name changes to match cntr_id. >> Changed the function names to >> rdtgroup_unassign_cntr >> More comments on commit log. >> >> v4: Added domain specific unassign feature. >> Few name changes. >> >> v3: Removed the static from the prototype of rdtgroup_unassign_abmc. >> The function is not called directly from user anymore. These >> changes are related to global assignment interface. >> >> v2: No changes. >> --- >> 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 4e8109dee174..cc832955b787 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -689,6 +689,8 @@ int resctrl_arch_assign_cntr(struct rdt_mon_domain >> *d, enum resctrl_event_id evt >> u32 rmid, u32 cntr_id, u32 closid, bool assign); >> int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, enum >> resctrl_event_id evtid); >> int rdtgroup_alloc_cntr(struct rdtgroup *rdtgrp, int index); >> +int rdtgroup_unassign_cntr(struct rdtgroup *rdtgrp, enum >> resctrl_event_id evtid); >> +void rdtgroup_free_cntr(struct rdt_resource *r, struct rdtgroup >> *rdtgrp, int index); >> 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 1ee91a7293a8..0c2215dbd497 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -1961,6 +1961,58 @@ int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, >> enum resctrl_event_id evtid) >> return 0; >> } >> +static int rdtgroup_mbm_cntr_test(struct rdt_resource *r, u32 cntr_id) > > Could "test" be replaced with something more specific about what is tested? > for example, "rdtgroup_mbm_cntr_is_assigned()" or something better? The Yes. We can do that. > function > looks like a good candidate for returning a bool. Sure. > > Is this function needed though? (more below) Yes. It is required. It is called from two places (rdtgroup_unassign_update and rdtgroup_unassign_cntr). We can open code in rdtgroup_unassign_cntr. But we can't do that in rdtgroup_unassign_update. But, I will check again for sure. > >> +{ >> + 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; >> +} >> + >> +/* Free the counter id after the event is unassigned */ >> +void rdtgroup_free_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp, >> + int index) >> +{ >> + /* Update the counter bitmap */ >> + if (!rdtgroup_mbm_cntr_test(r, rdtgrp->mon.cntr_id[index])) { >> + mbm_cntr_free(rdtgrp->mon.cntr_id[index]); >> + rdtgrp->mon.cntr_id[index] = MON_CNTR_UNSET; >> + } >> +} >> + >> +/* >> + * Unassign a hardware counter from the group and update all the domains >> + * in the group. >> + */ >> +int rdtgroup_unassign_cntr(struct rdtgroup *rdtgrp, enum >> resctrl_event_id evtid) >> +{ >> + struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> + struct rdt_mon_domain *d; >> + int index; >> + >> + index = mon_event_config_index_get(evtid); >> + if (index == INVALID_CONFIG_INDEX) >> + return -EINVAL; >> + >> + if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET) { >> + list_for_each_entry(d, &r->mon_domains, hdr.list) { >> + resctrl_arch_assign_cntr(d, evtid, rdtgrp->mon.rmid, >> + rdtgrp->mon.cntr_id[index], >> + rdtgrp->closid, false); >> + clear_bit(rdtgrp->mon.cntr_id[index], >> + d->mbm_cntr_map); >> + } >> + >> + /* Free the counter at group level */ >> + rdtgroup_free_cntr(r, rdtgrp, index); > > rdtgroup_free_cntr() is called right after the counter has been unassigned > from all domains. Will rdtgroup_mbm_cntr_test() thus not always return 0? > It seems unnecessary to have rdtgroup_mbm_cntr_test() and considering that, > rdtgroup_free_cntr() can just be open coded here? Yes. We can open code here. > >> + } >> + >> + return 0; >> +} >> + >> /* rdtgroup information files for one cache resource. */ >> static struct rftype res_common_files[] = { >> { > > Reinette > -- Thanks Babu Moger
