Re: [PATCH v9 3/9] x86/resctrl: Modify update_mba_bw() to use per ctrl_mon group event

["Luck, Tony" <tony.luck@intel.com>]

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@intel.com>
> > ---
> >   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().

See: https://lore.kernel.org/r/bb30835f-5be9-44b4-8544-2f528e7fc573@intel.com

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.

> > +
> >   	closid = rgrp->closid;
> >   	rmid = rgrp->mon.rmid;
> > -	idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> > -	pmbm_data = &dom_mbm->mbm_local[idx];
> > +	pmbm_data = get_mbm_state(dom_mbm, closid, rmid, evt_id);
> > +	if (WARN_ON_ONCE(!pmbm_data))
> > +		return;
> >   	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
> >   	if (!dom_mba) {
> > @@ -784,7 +795,9 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
> >   	 */
> >   	head = &rgrp->mon.crdtgrp_list;
> >   	list_for_each_entry(entry, head, mon.crdtgrp_list) {
> > -		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
> > +		cmbm_data = get_mbm_state(dom_mbm, entry->closid, entry->mon.rmid, evt_id);
> > +		if (WARN_ON_ONCE(!cmbm_data))
> > +			return;
> >   		cur_bw += cmbm_data->prev_bw;
> >   	}
> 
> -- 
> - Babu Moger

-Tony