linux-kernel - Re: [PATCH v4 12/31] fs/resctrl: Improve handling for events that can be read from any CPU

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: <5344df28-aea8-4f21-aad6-5587c4e21aaa@intel.com>
Date: Wed, 7 May 2025 20:54:13 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v4 12/31] fs/resctrl: Improve handling for events that can
 be read from any CPU

Hi Tony,

How about: "fs/resctrl: Handle events that can be read from any CPU"

On 4/28/25 5:33 PM, Tony Luck wrote:
> Resctrl file system code was built with the assumption that monitor
> events can only be read from a CPU in the cpumast_t set for each

cpumast_t -> cpumask_t

> domain. This was true for x86 events accessed with an MSR interface,
> but may not be true for other access methods such as MMIO.

Please separate context and problem description into separate paragraphs.

> 
> Add a flag to each instance of struct mon_evt that can be set by
> architecture code to indicate there is no restriction on which
> CPU can read the event counter.
> 
> Change struct mon_data and struct rmid_read to have a pointer to
> the struct mon_evt instead of the event id.
> 
> Add an extra argument to resctrl_enable_mon_event() so architecture
> code can indicate which events can be read on any CPU when enabling
> the event.
> 
> Bypass all the smp_call*() code for events that can be read on any CPU
> and call mon_event_count() directly from mon_event_read().
> 
> Skip checks in __mon_event_count() that the read is being done from
> a CPU in the correct domain or cache scope.

hmmm ... this patch is bundling quite a few changes. Most are related
to the stated goal but I think separating out the change to 
struct mon_data and struct rmid_read to have a pointer to struct mon_evt
will make it easier to recognize what it takes to support the
stated requirement of needing to support this new style of events.

...

> @@ -570,7 +575,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	struct rdt_domain_hdr *hdr;
>  	struct rmid_read rr = {0};
>  	struct rdt_l3_mon_domain *d;
> -	u32 resid, evtid, domid;
> +	struct mon_evt *evt;
> +	u32 resid, domid;

Please fix reverse fir.

>  	struct rdtgroup *rdtgrp;
>  	struct rdt_resource *r;
>  	struct mon_data *md;
> @@ -590,7 +596,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  
>  	resid = md->rid;
>  	domid = md->domid;
> -	evtid = md->evtid;
> +	evt = md->evt;
>  	r = resctrl_arch_get_resource(resid);
>  
>  	if (md->sum) {
> @@ -604,7 +610,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			if (d->ci->id == domid) {
>  				rr.ci = d->ci;
>  				mon_event_read(&rr, r, NULL, rdtgrp,
> -					       &d->ci->shared_cpu_map, evtid, false);
> +					       &d->ci->shared_cpu_map, evt, false);
>  				goto checkresult;
>  			}
>  		}
> @@ -621,7 +627,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			goto out;
>  		}
>  		d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> -		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
> +		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evt, false);
>  	}
>  
>  checkresult:
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 19cba29452b7..e903d3c076ee 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -365,19 +365,19 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	u64 tval = 0;
>  
>  	if (rr->first) {
> -		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
> -		m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
> +		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evt->evtid);
> +		m = get_mbm_state(rr->d, closid, rmid, rr->evt->evtid);
>  		if (m)
>  			memset(m, 0, sizeof(struct mbm_state));
>  		return 0;
>  	}
>  
>  	if (rr->d) {
> -		/* Reading a single domain, must be on a CPU in that domain. */
> -		if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> +		/* Reading a single domain, must usually be on a CPU in that domain. */

No need to use vague "usually" when it is very specific that it must be on a CPU in that
domain unless it is an event that can be read from any CPU.

> +		if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
>  			return -EINVAL;
>  		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> -						 rr->evtid, &tval, rr->arch_mon_ctx);
> +						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
>  		if (rr->err)
>  			return rr->err;
>  
> @@ -387,7 +387,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	}
>  
>  	/* Summing domains that share a cache, must be on a CPU for that cache. */

missed comment change

> -	if (!cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
> +	if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
>  		return -EINVAL;
>  
>  	/*

Reinette