Hi Tony, On 11/18/24 4:01 PM, Luck, Tony wrote: > On Fri, Nov 15, 2024 at 10:21:01AM -0600, Moger, Babu wrote: >> Hi Tony, > > Thanks for looking at this patch. > >> >> On 11/13/2024 6:17 PM, Tony Luck wrote: >>> Instead of hard-coding the memory bandwidth local event as the >>> input to the mba_sc feedback look, use the event that the user >>> configured for each ctrl_mon group. >>> >>> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx> >>> --- >>> arch/x86/kernel/cpu/resctrl/monitor.c | 23 ++++++++++++++++++----- >>> 1 file changed, 18 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >>> index 7ef1a293cc13..2176e355e864 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >>> @@ -752,20 +752,31 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm) >>> u32 closid, rmid, cur_msr_val, new_msr_val; >>> struct mbm_state *pmbm_data, *cmbm_data; >>> struct rdt_ctrl_domain *dom_mba; >>> + enum resctrl_event_id evt_id; >>> struct rdt_resource *r_mba; >>> - u32 cur_bw, user_bw, idx; >>> struct list_head *head; >>> struct rdtgroup *entry; >>> + u32 cur_bw, user_bw; >>> - if (!is_mbm_local_enabled()) >>> + if (!is_mbm_enabled()) >>> return; >>> r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; >>> + evt_id = rgrp->mba_mbps_event; >>> + >>> + if (WARN_ON_ONCE(!is_mbm_event(evt_id))) >>> + return; >> >> I feel this check is enough. >> >>> + if (WARN_ON_ONCE(evt_id == QOS_L3_MBM_LOCAL_EVENT_ID && !is_mbm_local_enabled())) >>> + return; >>> + if (WARN_ON_ONCE(evt_id == QOS_L3_MBM_TOTAL_EVENT_ID && !is_mbm_total_enabled())) >>> + return; >> >> These two checks are not necessary. You are already validating it while >> initializing(in patch 7). > > I added this in response to a comment on v7 from Reinette that evt_id > wasn't properly validated here - in conjuction with the change a few > lines earlier that relaxed the check for is_mbm_local_enabled() to > just is_mbm_enabled(). right that patch had an issue ... the "initialize" code hardcoded support to be r->membw.mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID; without any checking and then the handler used a relaxed check of is_mbm_enabled() On a system that only supports total MBM the is_mbm_enabled() check will pass while the event used will be local MBM. > > See: https://lore.kernel.org/r/bb30835f-5be9-44b4-8544-2f528e7fc573@xxxxxxxxx > > In theory all of these tests could be dropped. As you point out the > sanity checks are done higher in the call sequence. But some folks > like the "belt and braces" approach to such sanity checks. If that "higher in the call sequence" can be trusted, yes. That was not the case when I made those statements. Sprinkling WARN() that continues execution in a known bad state does not seem safe to me either. Reinette
