| 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: <aM2cctuXS8NBLeJ5@agluck-desk3>
Date: Fri, 19 Sep 2025 11:09:54 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: 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>, Chen Yu
<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
<patches@...ts.linux.dev>
Subject: Re: [PATCH v10 02/28] x86/resctrl: Move L3 initialization into new
helper function
On Thu, Sep 18, 2025 at 02:49:05PM -0700, Reinette Chatre wrote:
> Hi Tony,
>
> On 9/12/25 3:10 PM, Tony Luck wrote:
> > All resctrl monitor events are associated with the L3 resource, but
> > this is about to change.
>
> Please see Boris's feedback about changelogs in [1]. To address that,
> please rework the changelogs to not have so much copy&pasted context
> in patches.
I understand that Boris doesn't want to see large amounts of text copied
from the cover letter into each patch. But there is still a need to meet the
"Describe your problem" requirement for a good commit message.
Several of the prepatory patches in this series make changes to expand the
capabilities of fs/resctrl to handle monitor events from resources other
than RDT_RESOURCE_L3.
Is a single short sentence repeated in several patches "too much"?
>
> >
> > To prepare for additional types of monitoring domains, move open coded L3
> > resource monitoring domain initialization from domain_add_cpu_mon() into
> > a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().
> >
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> > arch/x86/kernel/cpu/resctrl/core.c | 57 +++++++++++++++++-------------
> > 1 file changed, 32 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 8be2619db2e7..055df4d406d0 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -496,37 +496,13 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
> > }
> > }
> >
> > -static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> > +static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
> > {
> > - int id = get_domain_id_from_scope(cpu, r->mon_scope);
> > - struct list_head *add_pos = NULL;
> > struct rdt_hw_mon_domain *hw_dom;
> > - struct rdt_domain_hdr *hdr;
> > struct rdt_mon_domain *d;
> > struct cacheinfo *ci;
> > int err;
> >
> > - lockdep_assert_held(&domain_list_lock);
> > -
> > - if (id < 0) {
> > - pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> > - cpu, r->mon_scope, r->name);
> > - return;
> > - }
> > -
> > - hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> > - if (hdr) {
> > - if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
> > - return;
> > - d = container_of(hdr, struct rdt_mon_domain, hdr);
> > -
> > - cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> > - /* Update the mbm_assign_mode state for the CPU if supported */
> > - if (r->mon.mbm_cntr_assignable)
> > - resctrl_arch_mbm_cntr_assign_set_one(r);
>
> Rebase error? Note the assignable counter initialization done on CPU online ...
Right. This code was lost in the rebase.
>
> > - return;
> > - }
> > -
> > hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
> > if (!hw_dom)
> > return;
> > @@ -565,6 +541,37 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> > }
> > }
> >
> > +static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> > +{
> > + int id = get_domain_id_from_scope(cpu, r->mon_scope);
> > + struct list_head *add_pos = NULL;
> > + struct rdt_domain_hdr *hdr;
> > +
> > + lockdep_assert_held(&domain_list_lock);
> > +
> > + if (id < 0) {
> > + pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> > + cpu, r->mon_scope, r->name);
> > + return;
> > + }
> > +
> > + hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> > + if (hdr) {
> > + cpumask_set_cpu(cpu, &hdr->cpu_mask);
> > +
>
> ... assignable counter initialization no longer done on CPU online.
>
> Looking closer the l3_mon_domain_setup() also now contains assignable counter
> initialization that is gated by a RDT_RESOURCE_L3 check. Considering the flow
> I think it may thus be simpler and consistent to not return here
> but instead have the additional initialization done in resource specific
> areas below.
Yes.
>
> > + return;
> > + }
> > +
> > + switch (r->rid) {
> > + case RDT_RESOURCE_L3:
>
> Something like:
> if (hdr) {
> /* do resource specific CPU initialization here */
> return;
> }
In this specific case the resource specific operation needs to happen
on every CPU (it updates the per-CPU MSR_IA32_L3_QOS_EXT_CFG). So I
think the code fragment needs to be:
switch (r->rid) {
case RDT_RESOURCE_L3:
/* Update the mbm_assign_mode state for the CPU if supported */
if (r->mon.mbm_cntr_assignable)
resctrl_arch_mbm_cntr_assign_set_one(r);
if (!hdr)
l3_mon_domain_setup(cpu, id, r, add_pos);
break;
>
> > + l3_mon_domain_setup(cpu, id, r, add_pos);
> > + break;
> > + default:
> > + pr_warn_once("Unknown resource rid=%d\n", r->rid);
> > + break;
> > + }
> > +}
> > +
> > static void domain_add_cpu(int cpu, struct rdt_resource *r)
> > {
> > if (r->alloc_capable)
>
> Reinette
-Tony
Powered by blists - more mailing lists