| 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: <b0a744ab-cf41-4db6-b0db-3a609b0f0256@intel.com>
Date: Fri, 25 Jul 2025 16:25:49 -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 v7 09/31] x86,fs/resctrl: Use struct rdt_domain_hdr
instead of struct rdt_mon_domain
Hi Tony,
On 7/11/25 4:53 PM, Tony Luck wrote:
> Historically all monitoring events have been associated with
> the L3 resource and it made sense to use the L3 specific "struct
> rdt_mon_domain *" arguments to functions manipulating domains.
Above is context describing current implementation so can be in present
tense. Needs imperative tone, eg. "All monitoring events are associated ..."
But
> the addition of monitor events tied to other resources changes this
> assumption.
>
> To enable enumeration of domains for events in other resources, change
> the calling sequence to use the generic struct rdt_domain_hdr for domain
> addition and deletion to preserve as much common code as possible.
>
> Same change to allow reading events in other resources. In this case
> the code flow passes from mon_event_read() via smp_call*() eventually
> to __mon_event_count() so the rmid_read::d field is replaced with
> the new rmid_read::hdr field.
>
> The mon_data structure is unchanged, but documentation is updated
> to note that mon_data::sum is only used for RDT_RESOURCE_L3.
Needs imperative tone.
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
...
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -159,7 +159,7 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
> break;
>
> entry = __rmid_entry(idx);
> - if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid,
> + if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid, entry->rmid,
> QOS_L3_OCCUP_EVENT_ID, &val,
> arch_mon_ctx)) {
> rmid_dirty = true;
> @@ -365,19 +365,23 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> int err, ret;
> 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);
> + if (rr->r->rid == RDT_RESOURCE_L3 && rr->first) {
> + if (WARN_ON_ONCE(!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN,
This seems like a doube WARN_ON_ONCE() considering the one within domain_header_is_valid().
> + RDT_RESOURCE_L3)))
> + return -EINVAL;
> + d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> + resctrl_arch_reset_rmid(rr->r, d, closid, rmid, rr->evtid);
> + m = get_mbm_state(d, closid, rmid, rr->evtid);
> if (m)
> memset(m, 0, sizeof(struct mbm_state));
> return 0;
> }
>
> - if (rr->d) {
> + if (rr->hdr) {
> /* Reading a single domain, must be on a CPU in that domain. */
> - if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> + if (!cpumask_test_cpu(cpu, &rr->hdr->cpu_mask))
> return -EINVAL;
> - rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> + rr->err = resctrl_arch_rmid_read(rr->r, rr->hdr, closid, rmid,
> rr->evtid, &tval, rr->arch_mon_ctx);
> if (rr->err)
> return rr->err;
> @@ -387,6 +391,9 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> return 0;
> }
>
> + if (WARN_ON_ONCE(rr->r->rid != RDT_RESOURCE_L3))
> + return -EINVAL;
> +
> /* Summing domains that share a cache, must be on a CPU for that cache. */
> ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
> if (!ci || ci->id != rr->ci_id)
> @@ -403,7 +410,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
> if (d->ci_id != rr->ci_id)
> continue;
> - err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
> + err = resctrl_arch_rmid_read(rr->r, &d->hdr, closid, rmid,
> rr->evtid, &tval, rr->arch_mon_ctx);
> if (!err) {
> rr->val += tval;
> @@ -432,9 +439,13 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> static void mbm_bw_count(u32 closid, u32 rmid, struct rmid_read *rr)
> {
> u64 cur_bw, bytes, cur_bytes;
> + struct rdt_mon_domain *d;
> struct mbm_state *m;
>
> - m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
> + if (WARN_ON_ONCE(domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3)))
> + return;
Double WARN_ON_ONCE()?
> + d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> + m = get_mbm_state(d, closid, rmid, rr->evtid);
> if (WARN_ON_ONCE(!m))
> return;
>
...
> @@ -3065,26 +3078,38 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
> if (ret)
> return ret;
>
> - if (!do_sum && resctrl_is_mbm_event(mevt->evtid))
> - mon_event_read(&rr, r, d, prgrp, &d->hdr.cpu_mask, mevt->evtid, true);
> + 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->evtid, true);
> }
>
> return 0;
> }
>
> static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> - struct rdt_mon_domain *d,
> + struct rdt_domain_hdr *hdr,
> struct rdt_resource *r, struct rdtgroup *prgrp)
> {
> struct kernfs_node *kn, *ckn;
> + bool snc_mode = false;
> + int domid = hdr->id;
> char name[32];
> - bool snc_mode;
> int ret = 0;
>
> lockdep_assert_held(&rdtgroup_mutex);
>
> - snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> - sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> + if (r->rid == RDT_RESOURCE_L3) {
> + if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> + return -EINVAL;
The domain header check only seems necessary when needing to run the container_of()
below that depends on SNC mode so can be moved into the if() below?
> + snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> + if (snc_mode) {
> + struct rdt_mon_domain *d;
> +
> + d = container_of(hdr, struct rdt_mon_domain, hdr);
> + domid = d->ci_id;
> + }
> + }
> + sprintf(name, "mon_%s_%02d", r->name, domid);
> +
> kn = kernfs_find_and_get(parent_kn, name);
> if (kn) {
> /*
Reinette
Powered by blists - more mailing lists