Hi Reinette, On 8/16/2024 4:41 PM, 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 patch is a mix of resctrl fs and arch code, could each piece please > be desribed clearly? I will separate them. That is probably better. > >> 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. >> >> Counters are configured by writing to L3_QOS_ABMC_CFG MSR and >> specifying the counter id, bandwidth source, and bandwidth types. >> >> Provide the interface to assign the counter ids to RMID. >> >> 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_alloc() from this patch to keep fs and arch code >> separate. >> Added code to update the counter assignment at domain level. >> >> v5: Few name changes to match cntr_id. >> Changed the function names to >> rdtgroup_assign_cntr >> resctr_arch_assign_cntr >> More comments on commit log. >> Added function summary. >> >> v4: Commit message update. >> User bitmap APIs where applicable. >> Changed the interfaces considering MPAM(arm). >> Added domain specific assignment. >> >> v3: Removed the static from the prototype of rdtgroup_assign_abmc. >> The function is not called directly from user anymore. These >> changes are related to global assignment interface. >> >> v2: Minor text changes in commit message. >> --- >> arch/x86/kernel/cpu/resctrl/internal.h | 4 ++ >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 97 ++++++++++++++++++++++++++ >> 2 files changed, 101 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h >> b/arch/x86/kernel/cpu/resctrl/internal.h >> index d93082b65d69..4e8109dee174 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -685,6 +685,10 @@ int mbm_cntr_alloc(struct rdt_resource *r); >> void mbm_cntr_free(u32 cntr_id); >> void resctrl_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom); >> unsigned int mon_event_config_index_get(u32 evtid); >> +int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, enum >> resctrl_event_id evtid, >> + 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); >> 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 60696b248b56..1ee91a7293a8 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -1864,6 +1864,103 @@ static ssize_t >> mbm_local_bytes_config_write(struct kernfs_open_file *of, >> return ret ?: nbytes; >> } >> +static void rdtgroup_abmc_cfg(void *info) > > This has nothing to do with a resctrl group (arch code has no insight > into the groups anyway). > Maybe an arch specific name like "resctrl_abmc_config_one_amd()" to > match earlier > "resctrl_abmc_set_one_amd()"? > Sure. > >> +{ >> + u64 *msrval = info; >> + >> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *msrval); >> +} >> + >> +/* >> + * Send an IPI to the domain to assign the counter id to RMID. >> + */ >> +int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, enum >> resctrl_event_id evtid, >> + u32 rmid, u32 cntr_id, u32 closid, bool assign) >> +{ >> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d); >> + union l3_qos_abmc_cfg abmc_cfg = { 0 }; >> + struct arch_mbm_state *arch_mbm; >> + >> + abmc_cfg.split.cfg_en = 1; >> + abmc_cfg.split.cntr_en = assign ? 1 : 0; >> + abmc_cfg.split.cntr_id = cntr_id; >> + abmc_cfg.split.bw_src = rmid; >> + >> + /* Update the event configuration from the domain */ >> + if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) { >> + abmc_cfg.split.bw_type = hw_dom->mbm_total_cfg; >> + arch_mbm = &hw_dom->arch_mbm_total[rmid]; >> + } else { >> + abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg; >> + arch_mbm = &hw_dom->arch_mbm_local[rmid]; >> + } >> + >> + smp_call_function_any(&d->hdr.cpu_mask, rdtgroup_abmc_cfg, >> &abmc_cfg, 1); >> + >> + /* >> + * Reset the architectural state so that reading of hardware >> + * counter is not considered as an overflow in next update. >> + */ >> + if (arch_mbm) >> + memset(arch_mbm, 0, sizeof(struct arch_mbm_state)); >> + >> + return 0; >> +} >> + >> +/* Allocate a new counter id if the event is unassigned */ >> +int rdtgroup_alloc_cntr(struct rdtgroup *rdtgrp, int index) >> +{ >> + struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> + int cntr_id; >> + >> + /* Nothing to do if event has been assigned already */ >> + if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET) { >> + rdt_last_cmd_puts("ABMC counter is assigned already\n"); > > This is resctrl fs code. Please replace the arch specific messages > ("ABMC") with resctrl fs terms. Sure. > >> + return 0; >> + } >> + >> + /* >> + * Allocate a new counter id and update domains >> + */ >> + cntr_id = mbm_cntr_alloc(r); >> + if (cntr_id < 0) { >> + rdt_last_cmd_puts("Out of ABMC counters\n"); > > here also. Sure. > >> + return -ENOSPC; >> + } >> + >> + rdtgrp->mon.cntr_id[index] = cntr_id; >> + >> + return 0; >> +} >> + >> +/* >> + * Assign a hardware counter to the group and assign the counter >> + * all the domains in the group. It will try to allocate the mbm >> + * counter if the counter is available. >> + */ >> +int rdtgroup_assign_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); > > After going through MPAM series this no longer looks correct. As the > name of this > function implies this is an index unique to the monitor event > configuration feature > and as the MPAM series highlights, it is unique to the architecture, not > something > that is visible to resctrl fs. resctrl fs uses the event IDs and it is > only when the > fs makes a request to the architecture that this translation comes into > play. > > With this change, what is the architecture specific "mon event config > index" now > becomes part of resctrl fs used for something totally different from mon > event > configuration. > > I think we should separate this to make sure we distinguish between an > architectural > translation and a resctrl fs translation, the array index is not the > same as the architecture > specific "mov event config index". > > How about we start with something simple that is defined by resctrl fs? > for example: > #define MBM_EVENT_ARRAY_INDEX(_event) (_event - 2) Yes. Good point. We can do that. > > >> + if (index == INVALID_CONFIG_INDEX) >> + return -EINVAL; >> + >> + if (rdtgroup_alloc_cntr(rdtgrp, index)) >> + return -EINVAL; >> + > > hmmm ... so rdtgroup_alloc_cntr() returns 0 if the counter is assigned > already, and > in this case the configuration is done again even if counter was already > assigned. > Is this intended? I didn't think thru this. Yea. It is not required as far as I can see. Will address it. > > rdtgroup_assign_cntr() seems to be almost identical to > rdtgroup_assign_update() > that has protection against the above from happening. It looks like > these two > functions can be merged into one? Yes. We can do that. > >> + list_for_each_entry(d, &r->mon_domains, hdr.list) { >> + resctrl_arch_assign_cntr(d, evtid, rdtgrp->mon.rmid, >> + rdtgrp->mon.cntr_id[index], > > There currently seems to be a mismatch between functions needing to > access this ID directly as above in some cases while also needing to > use helpers like rdtgroup_alloc_cntr(). I think I need to merge rdtgroup_assign_cntr(), rdtgroup_assign_update() and rdtgroup_alloc_cntr. It will probably make it clear. > > Also, as James indicated, resctrl_arch_assign_cntr() may fail on Arm > so this needs error checking even though the x86 implementation always > returns success. Sure. Will do. > >> + rdtgrp->closid, true); >> + set_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map); >> + } >> + >> + return 0; >> +} >> + >> /* rdtgroup information files for one cache resource. */ >> static struct rftype res_common_files[] = { >> { > > Reinette > Thanks - Babu Moger
