| 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: <da51ba61-4ff0-4db4-a55f-743f6a3ea7da@intel.com>
Date: Fri, 18 Apr 2025 15:54:02 -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>
CC: <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 10/26] fs/resctrl: Improve handling for events that can
be read from any CPU
Hi Tony,
On 4/7/25 4:40 PM, Tony Luck wrote:
> Add a flag to each instance of struct mon_event to indicate that there
> is no need for cross-processor interrupts to read this event from a CPU
> in a specific rdt_mon_domain.
>
> The flag is copied to struct mon_data for ease of access when a user
Copy the flag ...
> reads an event file invoking rdtgroup_mondata_show().
>
> Copied again into struct rmid_read in mon_event_read() for use by
> sanity checks in __mon_event_count().
>
> When the flag is set allow choice from cpu_online_mask. This makes the
> smp_call*() functions default to the current CPU.
Please use imperative tone.
>
> Suggested-by: James Morse <james.morse@....com>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> fs/resctrl/internal.h | 8 +++++++-
> fs/resctrl/ctrlmondata.c | 10 +++++++---
> fs/resctrl/monitor.c | 4 ++--
> fs/resctrl/rdtgroup.c | 1 +
> 4 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 08dbf89939ac..74a77794364d 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -72,6 +72,7 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
> * @evtid: event id
> * @name: name of the event
> * @configurable: true if the event is configurable
> + * @any_cpu: true if this event can be read from any CPU
> * @list: entry in &rdt_resource->evt_list
> */
> struct mon_evt {
> @@ -79,6 +80,7 @@ struct mon_evt {
> enum resctrl_res_level rid;
> char *name;
> bool configurable;
> + bool any_cpu;
> struct list_head list;
> };
>
> @@ -93,6 +95,7 @@ struct mon_evt {
> * the event file belongs. When @sum is one this
> * is the id of the L3 cache that all domains to be
> * summed share.
> + * @any_cpu: true if this event can be read from any CPU
> *
> * Stored in the kernfs kn->priv field, readers and writers must hold
> * rdtgroup_mutex.
> @@ -103,6 +106,7 @@ struct mon_data {
> enum resctrl_event_id evtid;
> unsigned int sum;
> unsigned int domid;
> + bool any_cpu;
> };
>
> /**
> @@ -115,6 +119,7 @@ struct mon_data {
> * domains in @r sharing L3 @ci.id
> * @evtid: Which monitor event to read.
> * @first: Initialize MBM counter when true.
> + * @any_cpu: When true read can be executed on any CPU.
> * @ci: Cacheinfo for L3. Only set when @d is NULL. Used when summing domains.
> * @err: Error encountered when reading counter.
> * @val: Returned value of event counter. If @rgrp is a parent resource group,
> @@ -129,6 +134,7 @@ struct rmid_read {
> struct rdt_mon_domain *d;
> enum resctrl_event_id evtid;
> bool first;
> + bool any_cpu;
> struct cacheinfo *ci;
> int err;
> u64 val;
Duplicating the same property across three structures does not look right. It looks to
me that struct mon_evt should be the "source of truth" for any event and that
these other structures can point to it instead of copying the data?
> @@ -358,7 +364,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
> - cpumask_t *cpumask, int evtid, int first);
> + const cpumask_t *cpumask, int evtid, int first);
>
> int resctrl_mon_resource_init(void);
>
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index 0c245af0ff42..cd77960657f0 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -525,7 +525,7 @@ struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h, int id,
>
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
> - cpumask_t *cpumask, int evtid, int first)
> + const cpumask_t *cpumask, int evtid, int first)
> {
> int cpu;
>
> @@ -571,6 +571,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> u32 resid, evtid, domid;
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> + const cpumask_t *mask;
> struct mon_data *md;
> int ret = 0;
>
> @@ -589,6 +590,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> resid = md->rid;
> domid = md->domid;
> evtid = md->evtid;
> + rr.any_cpu = md->any_cpu;
> r = resctrl_arch_get_resource(resid);
>
> if (md->sum) {
> @@ -601,8 +603,9 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> list_for_each_entry(d, &r->mon_domains, hdr.list) {
> if (d->ci->id == domid) {
> rr.ci = d->ci;
> + mask = md->any_cpu ? cpu_online_mask : &d->ci->shared_cpu_map;
> mon_event_read(&rr, r, NULL, rdtgrp,
> - &d->ci->shared_cpu_map, evtid, false);
> + mask, evtid, false);
> goto checkresult;
> }
> }
> @@ -619,7 +622,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> goto out;
> }
> d = container_of(hdr, struct rdt_mon_domain, hdr);
> - mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
> + mask = md->any_cpu ? cpu_online_mask : &d->hdr.cpu_mask;
> + mon_event_read(&rr, r, d, rdtgrp, mask, evtid, false);
I do not think this accomplishes the goal of this patch. Looking at mon_event_read() it calls
cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU) before any of the smp_*() calls.
cpumask_any_housekeeping()
{
...
if (exclude_cpu == RESCTRL_PICK_ANY_CPU)
cpu = cpumask_any(mask);
...
}
cpumask_any() is just cpumask_first() so it will pick the first CPU in the
online mask that may not be the current CPU.
fwiw ... there are some optimizations planned in this area that I have not yet studied:
https://lore.kernel.org/lkml/20250407153856.133093-1-yury.norov@gmail.com/
Reinette
Powered by blists - more mailing lists