Hi Tony, On 11/18/24 5:44 PM, Luck, Tony wrote: > On Mon, Nov 18, 2024 at 04:51:38PM -0800, Reinette Chatre wrote: >> 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. > > In the v9 series I believe all the necessary checks are made outside > of the update_mba_bw() function itself. > > update_mba_bw() is only called when is_mba_sc() returns true. Which > is the value of: > rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc > which can only be set if mbm is enabled. At this point in series mba_sc can still only be enabled if local MBM is enabled. > > So instead of changing the check from is_mbm_local_enabled() to > is_mbm_enabled() it could be deleted. Perhaps ... at this point in series without guidance from changelogs I am forced to dig through layers to deciper what patches aim to do, how they go about it, and how complete solution is built using these individual cryptic pieces. > rgrp->mba_mbps_event can only be set to QOS_L3_MBM_LOCAL_EVENT_ID > until patch 7 when the user can select QOS_L3_MBM_TOTAL_EVENT_ID > or patch 8 when the initiialization code can pick TOTAL on systems > that don't support LOCAL. > > So all three of the WARN_ON_ONCE() calls are unnecessary. > > Should I drop all these checks in v10? I am still trying to decipher this code and will aim to address this. Reinette
