| 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: <aORMZ6NUXMpECHU6@agluck-desk3>
Date: Mon, 6 Oct 2025 16:10:31 -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 v11 20/31] fs/resctrl: Refactor Sub-NUMA Cluster (SNC) in
mkdir/rmdir code flow
On Fri, Oct 03, 2025 at 04:58:45PM -0700, Reinette Chatre wrote:
> Hi Tony,
>
> On 9/25/25 1:03 PM, Tony Luck wrote:
> > SNC is only present in the RDT_RESOURCE_L3 domain.
> >
> > Refactor code that makes and removes directories under "mon_data" to
>
> "makes and removes directories" -> "makes and removes SNC directories"?
>
> > special case the L3 resource.
>
> Why?
>
> >
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> > fs/resctrl/rdtgroup.c | 50 +++++++++++++++++++++++++++----------------
> > 1 file changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> > index 6e8937f94e7a..cab5cb9e6c93 100644
> > --- a/fs/resctrl/rdtgroup.c
> > +++ b/fs/resctrl/rdtgroup.c
> > @@ -3155,6 +3155,7 @@ static void mon_rmdir_one_subdir(struct kernfs_node *pkn, char *name, char *subn
> > return;
> > kernfs_put(kn);
> >
> > + /* Subdirectories are only present on SNC enabled systems */
> > if (kn->dir.subdirs <= 1)
> > kernfs_remove(kn);
> > else
> > @@ -3171,19 +3172,24 @@ static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> > struct rdt_domain_hdr *hdr)
> > {
> > struct rdtgroup *prgrp, *crgrp;
> > - struct rdt_l3_mon_domain *d;
> > + int domid = hdr->id;
> > char subname[32];
> > - bool snc_mode;
> > char name[32];
> >
> > - if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > - return;
> > + if (r->rid == RDT_RESOURCE_L3) {
> > + struct rdt_l3_mon_domain *d;
> >
> > - d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > - snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > - sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> > - if (snc_mode)
> > - sprintf(subname, "mon_sub_%s_%02d", r->name, d->hdr.id);
> > + if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > + return;
> > +
> > + d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > + /* SNC mode? */
> > + if (r->mon_scope == RESCTRL_L3_NODE) {
> > + domid = d->ci_id;
> > + sprintf(subname, "mon_sub_%s_%02d", r->name, hdr->id);
> > + }
> > + }
> > + sprintf(name, "mon_%s_%02d", r->name, domid);
> >
> > list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> > mon_rmdir_one_subdir(prgrp->mon.mon_data_kn, name, subname);
> > @@ -3213,7 +3219,7 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_domain_hdr *hdr,
> > if (ret)
> > return ret;
> >
> > - if (!do_sum && resctrl_is_mbm_event(mevt->evtid))
> > + if (r->rid == RDT_RESOURCE_L3 && !do_sum && resctrl_is_mbm_event(mevt->evtid))
> > mon_event_read(&rr, r, hdr, prgrp, &hdr->cpu_mask, mevt, true);
> > }
> >
> > @@ -3225,19 +3231,27 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> > struct rdt_resource *r, struct rdtgroup *prgrp)
> > {
> > struct kernfs_node *kn, *ckn;
> > - struct rdt_l3_mon_domain *d;
> > + bool snc_mode = false;
> > + int domid = hdr->id;
> > char name[32];
> > - bool snc_mode;
> > int ret = 0;
> >
> > lockdep_assert_held(&rdtgroup_mutex);
> >
> > - if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > - return -EINVAL;
> > + if (r->rid == RDT_RESOURCE_L3) {
> > + snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > + if (snc_mode) {
> > + struct rdt_l3_mon_domain *d;
> > +
> > + if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > + return -EINVAL;
> > +
> > + d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > + domid = d->ci_id;
> > + }
> > + }
> > + sprintf(name, "mon_%s_%02d", r->name, domid);
> >
> > - d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > - snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > - sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> > kn = kernfs_find_and_get(parent_kn, name);
> > if (kn) {
> > /*
> > @@ -3253,7 +3267,7 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> > ret = rdtgroup_kn_set_ugid(kn);
> > if (ret)
> > goto out_destroy;
> > - ret = mon_add_all_files(kn, hdr, r, prgrp, hdr->id, snc_mode);
> > + ret = mon_add_all_files(kn, hdr, r, prgrp, domid, snc_mode);
> > if (ret)
> > goto out_destroy;
> > }
>
> mkdir_mondata_subdir(), similar to __mon_event_count(), is now unreasonably
> complicated. Just like in that earlier change this inconsistently adds
> RDT_RESOURCE_L3 checks, not to separate L3 code but instead to benefit PERF_PKG
> enabling to reach the handful of lines needed by it.
> Here too I think the best way forward is to split mkdir_mondata_subdir().
>
> rmdir_mondata_subdir_allrdtgrp() may also do with a split ... most of the
> code within it is dedicated to SNC and mon_rmdir_one_subdir() only exists
> because of SNC ... any other usage can just call kernfs_remove_by_name(), no?
>
> SNC is already complicated enabling and I think that PERF_PKG trying to wedge
> itself into that is just too confusing. I expect separating this should simplify
> this a lot.
Ok. Splitting these makes sense. I'm terrible at naming. So I
tentatively have:
static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
struct rdt_domain_hdr *hdr,
struct rdt_resource *r, struct rdtgroup *prgrp)
{
lockdep_assert_held(&rdtgroup_mutex);
if (r->mon_scope == RESCTRL_L3_NODE)
return mkdir_mondata_subdir_snc(parent_kn, hdr, r, prgrp);
else
return mkdir_mondata_subdir_normal(parent_kn, hdr, r, prgrp);
}
and:
static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
struct rdt_domain_hdr *hdr)
{
if (r->mon_scope == RESCTRL_L3_NODE)
rmdir_mondata_subdir_allrdtgrp_snc(r, hdr);
else
rmdir_mondata_subdir_allrdtgrp_normal(r, hdr);
}
Better suggestions gratefully accepted.
>
> Reinette
>
Powered by blists - more mailing lists