Hi Reinette, On 7/12/24 17:16, Reinette Chatre wrote: > Hi Babu, > > On 7/3/24 2:48 PM, Babu Moger wrote: >> Provide the interface to list the monitor states of all the resctrl >> groups in ABMC mode. >> >> Example: >> $cat /sys/fs/resctrl/info/L3_MON/mbm_control >> >> List follows the following format: >> >> "<CTRL_MON group>/<MON group>/<domain_id>=<flags>" >> >> Format for specific type of groups: >> >> - Default CTRL_MON group: >> "//<domain_id>=<flags>" >> >> - Non-default CTRL_MON group: >> "<CTRL_MON group>//<domain_id>=<flags>" >> >> - Child MON group of default CTRL_MON group: >> "/<MON group>/<domain_id>=<flags>" >> >> - Child MON group of non-default CTRL_MON group: >> "<CTRL_MON group>/<MON group>/<domain_id>=<flags>" >> >> >> Flags can be one of the following: >> t MBM total event is enabled >> l MBM local event is enabled >> tl Both total and local MBM events are enabled >> _ None of the MBM events are enabled >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- >> v5: Replaced "assignment flags" with "flags". >> Changes related to mon structure. >> Changes related renaming the interface from mbm_assign_control to >> mbm_control. >> >> v4: Added functionality to query domain specific assigment in. >> rdtgroup_abmc_dom_state(). >> >> v3: New patch. >> Addresses the feedback to provide the global assignment interface. >> >> https://lore.kernel.org/lkml/c73f444b-83a1-4e9a-95d3-54c5165ee782@xxxxxxxxx/ >> --- >> Documentation/arch/x86/resctrl.rst | 54 ++++++++++ >> arch/x86/kernel/cpu/resctrl/monitor.c | 1 + >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 130 +++++++++++++++++++++++++ >> 3 files changed, 185 insertions(+) >> >> diff --git a/Documentation/arch/x86/resctrl.rst >> b/Documentation/arch/x86/resctrl.rst >> index 4c41c5622627..05fee779e109 100644 >> --- a/Documentation/arch/x86/resctrl.rst >> +++ b/Documentation/arch/x86/resctrl.rst >> @@ -304,6 +304,60 @@ with the following files: >> "num_mbm_cntrs": >> The number of monitoring counters available for assignment. >> +"mbm_control": >> + Available when ABMC features are supported. > > "Available when ABMC features are supported." can be dropped > Ok. Sure. >> + Reports the resctrl group and monitor status of each group. >> + >> + List follows the following format: >> + "<CTRL_MON group>/<MON group>/<domain_id>=<flags>" >> + >> + Format for specific type of grpups: > > grpups -> groups Sure. > >> + >> + * Default CTRL_MON group: >> + "//<domain_id>=<flags>" >> + >> + * Non-default CTRL_MON group: >> + "<CTRL_MON group>//<domain_id>=<flags>" >> + >> + * Child MON group of default CTRL_MON group: >> + "/<MON group>/<domain_id>=<flags>" >> + >> + * Child MON group of non-default CTRL_MON group: >> + "<CTRL_MON group>/<MON group>/<domain_id>=<flags>" >> + >> + Flags can be one of the following: >> + :: >> + >> + t MBM total event is enabled. >> + l MBM local event is enabled. >> + tl Both total and local MBM events are enabled. >> + _ None of the MBM events are enabled. >> + >> + Examples: >> + :: >> + >> + # mkdir /sys/fs/resctrl/mon_groups/child_default_mon_grp >> + # mkdir /sys/fs/resctrl/non_default_ctrl_mon_grp >> + # mkdir >> /sys/fs/resctrl/non_default_ctrl_mon_grp/mon_groups/child_non_default_mon_grp >> + >> + # cat /sys/fs/resctrl/info/L3_MON/mbm_control >> + non_default_ctrl_mon_grp//0=tl;1=tl; >> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl; >> + //0=tl;1=tl; >> + /child_default_mon_grp/0=tl;1=tl; >> + >> + There are four resctrl groups. All the groups have total and local >> events are >> + enabled on domain 0 and 1. > > "All the groups have total and local events are enabled" -> "All the > groups have total and local events enabled"? > Sure. >> + > > The text below seems to repeat ealier description. I can remove it. > >> + non_default_ctrl_mon_grp// - This is a non-default CTRL_MON group. >> + >> + non_default_ctrl_mon_grp/child_non_default_mon_grp/ - This is a >> child monitor >> + group of non-default CTRL_MON group. >> + >> + // - This is a default CTRL_MON group. >> + >> + /child_default_mon_grp/ - This is a child monitor group of default >> CTRL_MON group. >> + >> "max_threshold_occupancy": >> Read/write file provides the largest value (in >> bytes) at which a previously used LLC_occupancy >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c >> b/arch/x86/kernel/cpu/resctrl/monitor.c >> index b96b0a8bd7d3..684730f1a72d 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -1244,6 +1244,7 @@ int __init rdt_get_mon_l3_config(struct >> rdt_resource *r) >> r->mon.num_mbm_cntrs = 64; >> resctrl_file_fflags_init("num_mbm_cntrs", RFTYPE_MON_INFO); >> + resctrl_file_fflags_init("mbm_control", RFTYPE_MON_INFO); > > Shouldn't this file always be present? > This is only relevent when monitor assign features are supported. Having the file without the feature is not usefull. >> } >> } >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index d978668c8865..0de9f23d5389 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -944,6 +944,130 @@ static ssize_t rdtgroup_mbm_mode_write(struct >> kernfs_open_file *of, >> return ret ?: nbytes; >> } >> +static void rdtgroup_abmc_dom_cfg(void *info) >> +{ >> + u64 *msrval = info; >> + >> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *msrval); >> + rdmsrl(MSR_IA32_L3_QOS_ABMC_DSC, *msrval); >> +} >> + >> +/* >> + * Writing the counter id with CfgEn=0 on L3_QOS_ABMC_CFG and reading >> + * L3_QOS_ABMC_DSC back will return configuration of the counter >> + * specified. > > Can this be expanded to explain what the return values mean? Sure. Basically returns the counter id with its configuration. Will add few more details. > >> + */ >> +static int rdtgroup_abmc_dom_state(struct rdt_mon_domain *d, u32 cntr_id, >> + u32 rmid) >> +{ >> + union l3_qos_abmc_cfg abmc_cfg = { 0 }; >> + >> + abmc_cfg.split.cfg_en = 0; >> + abmc_cfg.split.cntr_id = cntr_id; >> + >> + smp_call_function_any(&d->hdr.cpu_mask, rdtgroup_abmc_dom_cfg, >> + &abmc_cfg, 1); >> + >> + if (abmc_cfg.split.cntr_en && abmc_cfg.split.bw_src == rmid) >> + return 0; >> + else >> + return -1; >> +} >> + >> +static char *rdtgroup_mon_state_to_str(struct rdtgroup *rdtgrp, >> + struct rdt_mon_domain *d, char *str) >> +{ >> + char *tmp = str; >> + int dom_state = ASSIGN_NONE; > > reverse fir Sure. > >> + >> + /* >> + * Query the monitor state for the domain. >> + * Index 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID >> + * Index 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID > > Why not use the helper? Yes. > >> + */ >> + if (rdtgrp->mon.cntr_id[0] != MON_CNTR_UNSET) >> + if (!rdtgroup_abmc_dom_state(d, rdtgrp->mon.cntr_id[0], >> rdtgrp->mon.rmid)) >> + dom_state |= ASSIGN_TOTAL; >> + >> + if (rdtgrp->mon.cntr_id[1] != MON_CNTR_UNSET) >> + if (!rdtgroup_abmc_dom_state(d, rdtgrp->mon.cntr_id[1], >> rdtgrp->mon.rmid)) >> + dom_state |= ASSIGN_LOCAL; >> + >> + switch (dom_state) { >> + case ASSIGN_NONE: >> + *tmp++ = '_'; >> + break; >> + case (ASSIGN_TOTAL | ASSIGN_LOCAL): >> + *tmp++ = 't'; >> + *tmp++ = 'l'; >> + break; >> + case ASSIGN_TOTAL: >> + *tmp++ = 't'; >> + break; >> + case ASSIGN_LOCAL: >> + *tmp++ = 'l'; >> + break; >> + default: >> + break; >> + } > > This switch statement does not scale. Adding new flags will be painful. > Can flags not > just incrementally be printed as learned from hardware with "_" printed as > last resort? > This would elimininate need for these "ASSIGN" flags. Let me try to understand this. You want to remove switch statement. if (rdtgrp->mon.cntr_id[0] != MON_CNTR_UNSET) if (!rdtgroup_abmc_dom_state(d, rdtgrp->mon.cntr_id[0], rdtgrp->mon.rmid)) *tmp++ = 't'; if (rdtgrp->mon.cntr_id[1] != MON_CNTR_UNSET) if (!rdtgroup_abmc_dom_state(d, rdtgrp->mon.cntr_id[1], rdtgrp->mon.rmid)) *tmp++ = 'l'; If none of these flags are available, then *tmp++ = '_'; Is that the idea? > >> + >> + *tmp = '\0'; >> + return str; >> +} >> + >> +static int rdtgroup_mbm_control_show(struct kernfs_open_file *of, >> + struct seq_file *s, void *v) >> +{ >> + struct rdt_resource *r = of->kn->parent->priv; >> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); >> + struct rdt_mon_domain *dom; >> + struct rdtgroup *rdtg; >> + int grp_default = 0; >> + char str[10]; >> + >> + if (!hw_res->abmc_enabled) { >> + rdt_last_cmd_puts("ABMC feature is not enabled\n"); >> + return -EINVAL; >> + } >> + >> + mutex_lock(&rdtgroup_mutex); >> + >> + list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) { >> + struct rdtgroup *crg; >> + >> + if (rdtg == &rdtgroup_default) { >> + grp_default = 1; >> + seq_puts(s, "//"); >> + } else { >> + grp_default = 0; >> + seq_printf(s, "%s//", rdtg->kn->name); >> + } > > Isn't the default resource group's name already empty string? That should > eliminate the need for this special handling, no? Yea. Let me try that. > >> + >> + list_for_each_entry(dom, &r->mon_domains, hdr.list) >> + seq_printf(s, "%d=%s;", dom->hdr.id, >> + rdtgroup_mon_state_to_str(rdtg, dom, str)); >> + seq_putc(s, '\n'); >> + >> + list_for_each_entry(crg, &rdtg->mon.crdtgrp_list, >> + mon.crdtgrp_list) { >> + if (grp_default) >> + seq_printf(s, "/%s/", crg->kn->name); >> + else >> + seq_printf(s, "%s/%s/", rdtg->kn->name, >> + crg->kn->name); >> + > > Same here .... with default group having name of empty string it can just be > printed directly, no? Yea. Let me try that. > >> + list_for_each_entry(dom, &r->mon_domains, hdr.list) >> + seq_printf(s, "%d=%s;", dom->hdr.id, >> + rdtgroup_mon_state_to_str(crg, dom, str)); >> + seq_putc(s, '\n'); >> + } >> + } >> + >> + mutex_unlock(&rdtgroup_mutex); >> + >> + return 0; >> +} >> + >> #ifdef CONFIG_PROC_CPU_RESCTRL >> /* >> @@ -2156,6 +2280,12 @@ static struct rftype res_common_files[] = { >> .kf_ops = &rdtgroup_kf_single_ops, >> .seq_show = rdtgroup_num_mbm_cntrs_show, >> }, >> + { >> + .name = "mbm_control", >> + .mode = 0444, >> + .kf_ops = &rdtgroup_kf_single_ops, >> + .seq_show = rdtgroup_mbm_control_show, >> + }, >> { >> .name = "cpus_list", >> .mode = 0644, > > Reinette > -- Thanks Babu Moger
