[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fbd49f1a-ddb2-45e2-b943-df43d2173503@intel.com>
Date: Fri, 3 Oct 2025 16:58:45 -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>, Chen Yu <yu.c.chen@...el.com>
CC: <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
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.
Reinette
Powered by blists - more mailing lists