| 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
| ||
|
Message-ID: <Zl5OmVWFpBtA509j@agluck-desk3.sc.intel.com>
Date: Mon, 3 Jun 2024 16:15:37 -0700
From: Tony Luck <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: Fenghua Yu <fenghua.yu@...el.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>, x86@...nel.org,
linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH v19 16/20] x86/resctrl: Make resctrl_arch_rmid_read()
handle sum over domains
On Thu, May 30, 2024 at 01:24:57PM -0700, Reinette Chatre wrote:
> Hi Tony,
>
> On 5/28/24 3:20 PM, Tony Luck wrote:
> > Legacy resctrl monitor files must provide the sum of event values across
> > all Sub-NUMA Cluster (SNC) domains that share an L3 cache instance.
> >
> > Rename the existing resctrl_arch_rmid_read() function as
> > resctrl_arch_rmid_read_one() (with some small refactoring to drop
> > arguments that are not needed.
>
> Missing closing ")".
Will add.
> >
> > Create a new resctrl_arch_rmid_read() that iterates across
> > domains when necessary. Pass a CPU number from the right domain to
> > resctrl_arch_rmid_read_one().
>
> "when necessary"? Can you elaborate?
> "Pass a CPU ..." that just describes code that can be learned from patch.
> Please describe the changes not the code.
Will re-write.
> >
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> > arch/x86/kernel/cpu/resctrl/monitor.c | 41 ++++++++++++++++++++-------
> > 1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> > index 5f89ed4823ee..c9dd6ec68fcd 100644
> > --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> > @@ -200,10 +200,9 @@ static inline struct rmid_entry *__rmid_entry(u32 idx)
> > * Caller is responsible to make sure execution running on a CPU in
> > * the domain to be read.
> > */
> > -static int logical_rmid_to_physical_rmid(int lrmid)
> > +static int logical_rmid_to_physical_rmid(int cpu, int lrmid)
> > {
> > struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> > - int cpu = smp_processor_id();
> > if (snc_nodes_per_l3_cache == 1)
> > return lrmid;
> > @@ -211,13 +210,13 @@ static int logical_rmid_to_physical_rmid(int lrmid)
> > return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->num_rmid;
> > }
> > -static int __rmid_read(u32 lrmid,
> > +static int __rmid_read(int cpu, u32 lrmid,
> > enum resctrl_event_id eventid, u64 *val)
> > {
> > u64 msr_val;
> > int prmid;
> > - prmid = logical_rmid_to_physical_rmid(lrmid);
> > + prmid = logical_rmid_to_physical_rmid(cpu, lrmid);
> > /*
>
> Passing CPU as parameter to __rmid_read(), which is run via IPI, really
> obfuscates the code. How about renaming it to "__rmid_read_phys()"
> and provide it the "physical RMID" as parameter to make it clear what it is
> doing?
> That would mean an extra call to determine "physical RMID" before calling
> __rmid_read_phys() but making it clear that it needs a physical RMID should
> help to explain what is going on.
>
>
> > * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured
> > * with a valid event code for supported resource type and the bits
> > @@ -269,7 +268,7 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
> > memset(am, 0, sizeof(*am));
> > /* Record any initial, non-zero count value. */
> > - __rmid_read(rmid, eventid, &am->prev_msr);
> > + __rmid_read(smp_processor_id(), rmid, eventid, &am->prev_msr);
> > }
> > }
> > @@ -298,9 +297,8 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
> > return chunks >> shift;
> > }
> > -int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> > - u32 unused, u32 rmid, enum resctrl_event_id eventid,
> > - u64 *val, bool sum, struct cacheinfo *ci, void *ignored)
> > +static int resctrl_arch_rmid_read_one(struct rdt_resource *r, struct rdt_mon_domain *d,
> > + int cpu, u32 rmid, enum resctrl_event_id eventid, u64 *val)
> > {
> > struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> > @@ -313,7 +311,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> > if (!cpumask_test_cpu(smp_processor_id(), &d->hdr.cpu_mask))
> > return -EINVAL;
> > - ret = __rmid_read(rmid, eventid, &msr_val);
> > + ret = __rmid_read(cpu, rmid, eventid, &msr_val);
> > if (ret)
> > return ret;
> > @@ -327,7 +325,30 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> > chunks = msr_val;
> > }
> > - *val = chunks * hw_res->mon_scale;
> > + *val += chunks * hw_res->mon_scale;
>
> The various new layers of indirection with SNC logic scattered between them
> makes this change hard to understand.
>
> > +
> > + return 0;
> > +}
> > +
> > +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> > + u32 unused, u32 rmid, enum resctrl_event_id eventid,
> > + u64 *val, bool sum, struct cacheinfo *ci, void *ignored)
>
> This is not architecture specific code.
Can you explain further? I've dropped the "sum" argument. As you pointed
out elsewhere this can be inferred from "d == NULL". But I do need the
cacheinfo information in resctrl_arch_rmid_read() to:
1) determine which domains to sum (those that match ci->id).
2) sanity check code is executing on a CPU in ci->shared_cpu_map.
> > +{
> > + int cpu = smp_processor_id();
> > + int ret;
> > +
> > + *val = 0;
> > + if (!sum)
> > + return resctrl_arch_rmid_read_one(r, d, cpu, rmid, eventid, val);
> > +
>
> /* SNC quirk that needs to be documented */
>
> > + list_for_each_entry(d, &r->mon_domains, hdr.list) {
> > + if (d->ci->id != ci->id)
> > + continue;
> > + cpu = cpumask_any(&d->hdr.cpu_mask);
> > + ret = resctrl_arch_rmid_read_one(r, d, cpu, rmid, eventid, val);
>
> The cpu parameter can be dropped, no? With the domain provided to resctrl_arch_rmid_read_one()
> it is not necessary to again split the SNC logic (in this case the "reading from any CPU
> in the cache domain is ok but still need accurate arch state") across multiple layers,
> just contain this in (documented portion of) resctrl_arch_rmid_read_one().
You are correct. I will drop the cpu argument.
> > + if (ret)
> > + return ret;
> > + }
> > return 0;
> > }
>
> Reinette
-Tony
Powered by blists - more mailing lists