Hi Reinette, On 6/13/24 20:49, Reinette Chatre wrote: > Hi Babu, > > On 5/24/24 5:23 AM, Babu Moger wrote: >> Hardware provides a limited number of ABMC counters. Once all the >> counters are exhausted, counters need to be freed for new assignments. >> >> Provide the interface to unassign the counter. > > Please write a proper changelog. This needs information that explains > what this patch does and why. Sure. Will elaborate on this. > >> >> 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). >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 >> --- >> 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 | 42 ++++++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h >> b/arch/x86/kernel/cpu/resctrl/internal.h >> index a88c8fc5e4df..e16244895350 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -660,6 +660,8 @@ void arch_domain_mbm_evt_config(struct rdt_hw_domain >> *hw_dom); >> int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid, >> u32 ctr_id, u32 closid, bool enable); >> int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid); >> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid); >> +void num_cntrs_free(u32 ctr_id); >> 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 48df76499a04..5ea1e58c7201 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -216,6 +216,11 @@ static int assign_cntrs_alloc(void) >> return ctr_id; >> } >> +void num_cntrs_free(u32 ctr_id) > > The name does not reflect what it does. It neither frees the "num_cntrs" > information > nor does it free "num_cntrs" of counters. How about "mon_cntr_free()"? Sure. If we change the name of num_cntrs to mbm_cntrs, then we can change to mbm_cntr_free(). Here is the comments on name change. https://lore.kernel.org/lkml/62fe683f-3a4c-4280-8770-d2aaff478d33@xxxxxxx/ > > >> +{ >> + __set_bit(ctr_id, &num_cntrs_free_map); >> +} >> + >> /** >> * rdtgroup_mode_by_closid - Return mode of resource group with closid >> * @closid: closid if the resource group >> @@ -1931,6 +1936,43 @@ int resctrl_grp_assign(struct rdtgroup *rdtgrp, >> u32 evtid) >> return 0; >> } >> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid) > > Same comment wrt namespace. Also this function needs a description. Sure. > >> +{ >> + struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> + struct rdt_domain *d; >> + u32 mon_state; >> + int index; >> + >> + index = mon_event_config_index_get(evtid); >> + if (index == INVALID_CONFIG_INDEX) { >> + pr_warn_once("Invalid event id %d\n", evtid); >> + return -EINVAL; >> + } >> + >> + if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) { >> + mon_state = ASSIGN_TOTAL; >> + } else if (evtid == QOS_L3_MBM_LOCAL_EVENT_ID) { >> + mon_state = ASSIGN_LOCAL; >> + } else { >> + rdt_last_cmd_puts("Invalid event id\n"); >> + return -EINVAL; >> + } >> + >> + if (rdtgrp->mon.mon_state & mon_state) { >> + list_for_each_entry(d, &r->domains, list) >> + resctrl_arch_assign(d, evtid, rdtgrp->mon.rmid, >> + rdtgrp->mon.ctr_id[index], >> + rdtgrp->closid, 0); >> + >> + /* Update the counter bitmap */ >> + num_cntrs_free(rdtgrp->mon.ctr_id[index]); >> + } >> + >> + rdtgrp->mon.mon_state &= ~mon_state; >> + >> + return 0; >> +} >> + >> /* rdtgroup information files for one cache resource. */ >> static struct rftype res_common_files[] = { >> { > > > Reinette > -- Thanks Babu Moger
