Hi Babu, On 10/29/24 4:21 PM, Babu Moger wrote: > Users can modify the configuration of assignable events. Whenever the > event configuration is updated, MBM assignments must be revised across > all monitor groups within the impacted domains. Please revisit the "Changelog" section in Documentation/process/maintainer-tip.rst > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > --- > v9: Again patch changed completely based on the comment. > https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@xxxxxxxxx/ > Introduced resctrl_mon_event_config_set to handle IPI. > But sending another IPI inside IPI causes problem. Kernel reports SMP > warning. So, introduced resctrl_arch_update_cntr() to send the command directly. I see ... the WARN is because there is a check whether IRQs are disabled before the check whether the function can be run locally. > > v8: Patch changed completely. > Updated the assignment on same IPI as the event is updated. > Could not do the way we discussed in the thread. > https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@xxxxxxx/ > Needed to figure out event type to update the configuration. > > v7: New patch to update the assignments. Missed it earlier. > --- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 +++++++++++++++++++++++--- > include/linux/resctrl.h | 3 +- > 2 files changed, 79 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 5b8bb8bd913c..7646d67ea10e 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of, > } > > struct mon_config_info { > + struct rdt_resource *r; > struct rdt_mon_domain *d; > u32 evtid; > u32 mon_config; > @@ -1735,26 +1736,28 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d, > return INVALID_CONFIG_VALUE; > } > > -void resctrl_arch_mon_event_config_set(void *info) > +void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d, > + enum resctrl_event_id eventid, u32 val) > { > - struct mon_config_info *mon_info = info; > struct rdt_hw_mon_domain *hw_dom; > unsigned int index; > > - index = mon_event_config_index_get(mon_info->evtid); > + index = mon_event_config_index_get(eventid); > if (index == INVALID_CONFIG_INDEX) > return; > > - wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0); > + wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0); > > - hw_dom = resctrl_to_arch_mon_dom(mon_info->d); > + hw_dom = resctrl_to_arch_mon_dom(d); > > - switch (mon_info->evtid) { > + switch (eventid) { > case QOS_L3_MBM_TOTAL_EVENT_ID: > - hw_dom->mbm_total_cfg = mon_info->mon_config; > + hw_dom->mbm_total_cfg = val; > break; > case QOS_L3_MBM_LOCAL_EVENT_ID: > - hw_dom->mbm_local_cfg = mon_info->mon_config; > + hw_dom->mbm_local_cfg = val; > + break; > + default: > break; > } > } > @@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of, > return 0; > } > > +static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index) > +{ > + struct rdtgroup *prgrp, *crgrp; > + > + /* Check if the cntr_id is associated to the event type updated */ > + list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) { > + if (prgrp->mon.cntr_id[index] == cntr_id) > + return prgrp; > + > + list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) { > + if (crgrp->mon.cntr_id[index] == cntr_id) > + return crgrp; > + } > + } > + > + return NULL; > +} > + > +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, > + enum resctrl_event_id evtid, u32 rmid, > + u32 closid, u32 cntr_id, u32 val) > +{ > + union l3_qos_abmc_cfg abmc_cfg = { 0 }; > + > + abmc_cfg.split.cfg_en = 1; > + abmc_cfg.split.cntr_en = 1; > + abmc_cfg.split.cntr_id = cntr_id; > + abmc_cfg.split.bw_src = rmid; > + abmc_cfg.split.bw_type = val; > + > + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full); Is it needed to create an almost duplicate function? What if instead only resctrl_arch_config_cntr() exists and it uses parameter to decide whether to call resctrl_abmc_config_one_amd() directly or via smp_call_function_any()? I think that should help to make clear how the code flows. Also note that this is an almost identical arch callback with no error return. I expect that building on existing resctrl_arch_config_cntr() will make things easier to understand. > +} > + > +static void resctrl_mon_event_config_set(void *info) > +{ > + struct mon_config_info *mon_info = info; > + struct rdt_mon_domain *d = mon_info->d; > + struct rdt_resource *r = mon_info->r; Note that local variable r is created here while the function is inconsistent by switching between using r and mon_info->r. > + struct rdtgroup *rdtgrp; > + unsigned int index; > + u32 cntr_id; > + > + resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config); > + > + if (!resctrl_arch_mbm_cntr_assign_enabled(r)) > + return; > + > + index = mon_event_config_index_get(mon_info->evtid); This is an AMD arch specific helper to know which offset of an MSR to use. It should not be used directly in resctrl fs code, this is what MBM_EVENT_ARRAY_INDEX was created for. Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used, within rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name. > + if (index == INVALID_CONFIG_INDEX) > + return; > + > + for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) { > + if (test_bit(cntr_id, d->mbm_cntr_map)) { > + rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index); > + if (rdtgrp) > + resctrl_arch_update_cntr(mon_info->r, d, > + mon_info->evtid, > + rdtgrp->mon.rmid, > + rdtgrp->closid, > + cntr_id, > + mon_info->mon_config); > + } > + } > +} Could you please add some function comments to explain the flow here? For example, what should reader consider if there is to rdtgroup found? Reinette
