Re: [RFC PATCH] x86,resctrl: Read event counters with an IPI when the read cannot block

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Shakeel,

On 10/20/26 8:13 AM, Shakeel Butt wrote:
> In the Meta fleet, readers of the resctrl mon_data files often sleep
> while holding rdtgroup_mutex. Over several hours, a lock profiler on
> several thousand hosts (almost all AMD) caught:
> 
>   - 96,368 reads that held rdtgroup_mutex and cpu_hotplug_lock for
>     17 ms on average and up to several seconds, mostly sleeping.
>   - Three different userspace applications [1, 2, 3] doing these reads.

This is very demanding usage of an interface that cannot and has never 
intended to be fast.

>   - Other readers and the MBM overflow worker waiting for the mutex.
> 
> These long stalls put gaps in the monitoring data for these
> applications.
> 
> The read has to run on a CPU in the L3 domain. mon_event_read() uses
> smp_call_on_cpu() for that. It queues work on that CPU and sleeps until
> a kworker runs it. When that CPU is busy, this takes a long time, and
> the mutex stays held all along.
> 
> Commit 09909e098113 ("x86/resctrl: Queue mon_event_read() instead of
> sending an IPI") did this for MPAM, whose reads may sleep. On x86 a
> read is just one WRMSR and one RDMSR and it don't need to sleep.
> 
> Add CONFIG_RESCTRL_MON_READ_IRQSAFE, which x86 selects. With it, read
> the counter with interrupts off: on the current CPU if it is a
> housekeeping (not nohz_full) CPU in the domain, otherwise with an IPI
> to one. If every CPU in the domain is nohz_full, keep the existing
> smp_call_function_any() fallback. MPAM and other architectures keep
> the old path. Peter Newman posted a similar change in 2024 [4].

Did you consider the thread that followed that proposal to learn why it did
not land? 

> 
> Results on a 316-CPU AMD EPYC with busy target CPUs, and readers that
> copy the fleet agents, per 60 s run:
> 
>                                        before    after
>   hold time per read, average          350 us    4 us
>   hold time per read, worst            3.4 ms    109 us
>   time off CPU while holding           9 s       0
>   one pass over 750 files, average     360 ms    7.4 ms

The numbers make the contention clear. If I got the math right the "after"
is still about 10us per file and with 4us spent holding rdtgroup_mutex there
is still about 6us per file spent outside resctrl. I expect this is
taken up by the syscalls, the various kernfs allocations to manage the read
cycle, also a seq_buf allocation of a page (just to return a few bytes of data).

I do not think the 750 file example accurately captures how "bad" things
can get. From what I can tell this proposal only *slightly* adjusts the limit
of how many monitor groups this deployment can support.

My guess is that the 750 file example looks like possibly about 25 monitor
groups with three events each monitored across 10 domains.

The "after" value indicates that this deployment can now support more monitoring
groups. For example, 250 monitor groups with a pass over the increased number
of files taking about 74ms.

AMD can support 4096 monitor groups though so while this change is undeniably
an improvement, it does not address the foundational limitations of this
interface and does not enable this deployment to use full capability of
the systems.

If the goal is indeed to obtain accurate monitoring data at regular
intervals when using the stated hardware at its full capacity then
I would suggest that you consider the per-group rate files that Peter
settled on per
https://lore.kernel.org/all/CALPaoCgpnVORZfbKVLXDFUZvv8jhpShHPzB3cwdLTZQH1o9ULw@mail.gmail.com/

I actually cannot remember seeing an update on that work after he posted that
message and my assumption has been that that is what they have been using since
then.

Essentially the solution builds on the monitoring data already collected by the
per-domain overflow handler, it turns that data into a rate that user space can
read on demand without any IPI.
 
> The cost: interrupts are off on the reading CPU for about 0.5 us per
> counter, and up to 131 us for a root group read with 128 MON groups.

128 MON groups sound low - it looks like AMD has 4096 possible MON groups.

Interrupting a monitored application with interrupts off for so long
is significant for a fleet whose product is the monitored application.

> A reader that runs every 100 ms used up to 29% more system time,

Is it really needed to get the data every 100ms? The overflow handler runs
every second and the data could just be queried from it.

> because it now spins for the IPI. A kernel with lockdep and
> DEBUG_ATOMIC_SLEEP gave no warnings while groups were created and
> removed and CPUs went offline under the readers.
> 
> Link: https://github.com/facebookincubator/below [1]
> Link: https://github.com/sched-ext/scx [2]
> Link: https://github.com/facebookincubator/dynolog [3]
> Link: https://lore.kernel.org/20241106154306.2721688-2-peternewman@google.com [4]
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> ---

...

As I understand this patch slightly moves the needle on how many monitoring
groups this deployment can support. I do not believe this patch enables this
deployment to support all monitoring groups possible. 

Please do consider per-group rate files as discussed in the thread around [4].

Even so, surely this patch improves performance under some scenarios. We could continue
to consider it if you still believe it useful.

> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index cafebdff70dc..c6fea953e962 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -601,10 +601,18 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  	 * MPAM's resctrl_arch_rmid_read() is unable to read the
>  	 * counters on some platforms if its called in IRQ context.
>  	 */
> -	if (tick_nohz_full_cpu(cpu))
> +	if (tick_nohz_full_cpu(cpu)) {
>  		smp_call_function_any(cpumask, mon_event_count, rr, 1);
> -	else
> +	} else if (IS_ENABLED(CONFIG_RESCTRL_MON_READ_IRQSAFE)) {

Please avoid a config option. This could be a per-resource property,
for example, rdt_resource::mon::read_irq_safe (I'll defer naming debates).
I'd recommend against an architectural helper like [4] since now that there
are multiple architectural drivers the scope of change will be significant since
it spans across branches that take different routes upstream.

> +		int this_cpu = raw_smp_processor_id();

hmmm ... this is meaningless without preemption disabled, no? Looks like this
uses raw_ to get around the CONFIG_DEBUG_PREEMPT check :(

On the face of this it seems to attempt a re-implementation of smp_call_function_any()?

> +
> +		if (cpumask_test_cpu(this_cpu, cpumask) &&
> +		    !tick_nohz_full_cpu(this_cpu))

Doesn't this "!tick_nohz_full_cpu(this_cpu)" check defeat one major goal of this
patch, which is to prevent an IPI if a reader pinned itself to the domain? A monitoring
data reader may very well pin itself to a CPU in the monitoring domain and be running
on a nohz_full CPU.

It looks like the rule should be "prefer the current CPU whenever it is in the domain,
nohz_full or not"?

On first glance I wonder if this could be re-ordered. I surely need to think about this
more but how about something like below:


	if (r->mon.read_irq_safe) {
                  /*
                   * Reading the counter does not sleep, it only has to run on a
                   * CPU in the domain. Prefer the current CPU when it is one:
                   * the counter can be read without an IPI and without
                   * disturbing any other CPU. A nohz_full current CPU is not
                   * avoided, the reasons to avoid nohz_full CPUs, interrupting
                   * user space and queueing work that may never run, do not
                   * apply to the CPU making this request.
                   *
                   * Preemption is disabled for the test because the answer is
                   * only valid while this task cannot migrate. It is enabled
                   * again before the IPI to let other work run while waiting.
                   */
                  cpu = get_cpu();
                  if (!cpumask_test_cpu(cpu, cpumask))
                          cpu = cpumask_any_housekeeping(cpumask,
                                                         RESCTRL_PICK_ANY_CPU);
                  put_cpu();

                  smp_call_function_single(cpu, mon_event_count, rr, 1);
                  goto out_ctx_free;
          }

          cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);		
	 ...


> +			cpu = this_cpu;
> +		smp_call_function_single(cpu, mon_event_count, rr, true);
> +	} else {
>  		smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
> +	}
>  
>  out_ctx_free:
>  	if (rr->arch_mon_ctx)
Reinette