linux-kernel - Re: [PATCH v9 3/9] x86/resctrl: Modify update_mba_bw() to use per ctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzvVSamxTo9QSWBU@agluck-desk3>
Date: Mon, 18 Nov 2024 16:01:13 -0800
From: "Luck, Tony" <tony.luck@...el.com>
To: babu.moger@....com
Cc: Fenghua Yu <fenghua.yu@...el.com>,
	Reinette Chatre <reinette.chatre@...el.com>,
	Peter Newman <peternewman@...gle.com>,
	Jonathan Corbet <corbet@....net>, x86@...nel.org,
	James Morse <james.morse@....com>,
	Jamie Iles <quic_jiles@...cinc.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	"Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v9 3/9] x86/resctrl: Modify update_mba_bw() to use per
 ctrl_mon group event

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@...el.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