[PATCH 1/2] selftests/resctrl: fastcat for benchmarking counter reads

[PATCH 1/2] selftests/resctrl: fastcat for benchmarking counter reads

[Peter Newman]

This patch is provided for reference and not intended for submission.

This test program is used to evaluate the cost of reading resctrl
counters. It is essentially a drop-in replacement for cat, allowing
presumably small files, like resctrl nodes, to be read repeatedly using
pread(). In contrast, repeated invocations of cat would open and close
the counter files, which is an order of magnitude more costly than the
actual counter file read operations.

For example, the steps below were used to determine the cost of reading
a large number of local or remote counters on a dual-socket AMD Zen2:

Bind to a CPU in L3 domain 6:

 # taskset -c 25 bash

Record the setup cost for the groups /sys/fs/resctrl/mon_groups/*:

 # cd /sys/fs/resctrl
 # FASTCAT_READ_COUNT=0 perf stat -r 1000 /tmp/fastcat mon_groups/*/mon_data/mon_L3_06/mbm_*

Measure local read cost:

 # perf stat -r 1000 /tmp/fastcat mon_groups/*/mon_data/mon_L3_06/mbm_*

Measure remote read cost:

 # perf stat -r 1000 /tmp/fastcat mon_groups/*/mon_data/mon_L3_07/mbm_*

Signed-off-by: Peter Newman <peternewman@google.com>
---
 tools/testing/selftests/resctrl/.gitignore |  1 +
 tools/testing/selftests/resctrl/Makefile   |  6 ++-
 tools/testing/selftests/resctrl/fastcat.c  | 63 ++++++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/resctrl/fastcat.c

diff --git a/tools/testing/selftests/resctrl/.gitignore b/tools/testing/selftests/resctrl/.gitignore
index ab68442b6bc8d..11a40e331f4ad 100644
--- a/tools/testing/selftests/resctrl/.gitignore
+++ b/tools/testing/selftests/resctrl/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 resctrl_tests
+fastcat
diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index f408bd6bfc3d4..c5ec1e8289390 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -3,10 +3,12 @@
 CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
 CFLAGS += $(KHDR_INCLUDES)
 
-TEST_GEN_PROGS := resctrl_tests
+TEST_GEN_PROGS := resctrl_tests fastcat
 
 LOCAL_HDRS += $(wildcard *.h)
 
 include ../lib.mk
 
-$(OUTPUT)/resctrl_tests: $(wildcard *.c)
+$(OUTPUT)/resctrl_tests: cache.c cat_test.c cmt_test.c fill_buf.c mba_test.c mbm_test.c resctrlfs.c resctrl_tests.c resctrl_val.c
+
+$(OUTPUT)/fastcat: fastcat.c
diff --git a/tools/testing/selftests/resctrl/fastcat.c b/tools/testing/selftests/resctrl/fastcat.c
new file mode 100644
index 0000000000000..ac7a56f82a7c2
--- /dev/null
+++ b/tools/testing/selftests/resctrl/fastcat.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int main(int argc, char **argv)
+{
+	int nfiles = argc - 1;
+	char *nread_str;
+	int nread;
+	int *fds;
+	int i, j;
+
+	nread_str = getenv("FASTCAT_READ_COUNT");
+	if (!nread_str)
+		nread = 1;
+	else
+		nread = atoi(nread_str);
+
+	if (nfiles < 1)
+		exit(1);
+
+	fds = malloc(sizeof(*fds) * (argc));
+	if (!fds) {
+		perror("malloc");
+		exit(1);
+	}
+
+	printf("opening %d files\n", nfiles);
+
+	for (i = 1; i < argc; i++) {
+		fds[i - 1] = open(argv[i], O_RDONLY);
+		if (fds[i - 1] < 0) {
+			perror(argv[i]);
+			exit(1);
+		}
+	}
+
+	printf("reading %d files %d times\n", nfiles, nread);
+
+	for (j = 0; j < nread; j++) {
+		for (i = 0; i < nfiles; i++) {
+			// Assumed to be large enough for any output of
+			// mbm_*_bytes
+			char buf[40];
+			ssize_t r;
+
+			r = pread(fds[i], buf, sizeof(buf), 0);
+			if (r < 0) {
+				perror(argv[i + 1]);
+				exit(1);
+			}
+		}
+	}
+
+	printf("closing %d files\n", nfiles);
+	for (i = 0; i < nfiles; i++)
+		close(fds[i]);
+
+	return 0;
+}

base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
-- 
2.47.0.199.ga7371fff76-goog

[PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Peter Newman]

Performance-conscious users may use threads bound to CPUs within a
specific monitoring domain to ensure that all bandwidth counters can be
read efficiently. The hardware counters are only accessible to CPUs
within the domain, so requests from CPUs outside the domain are
forwarded to a kernel worker or IPI handler, incurring a substantial
performance penalty on each read.  Recently, this penalty was observed
to be paid by local reads as well.

To support blocking implementations of resctrl_arch_rmid_read(),
mon_event_read() switched to smp_call_on_cpu() in most cases to read
event counters using a kernel worker thread. Unlike
smp_call_function_any(), which optimizes to a local function call when
the calling CPU is in the target cpumask, smp_call_on_cpu() queues the
work unconditionally.

Add a fast-path to ensure that requests bound to within the monitoring
domain are read using a simple function call into mon_event_count()
regardless of whether all CPUs in the target domain are using nohz_full.

This is significant when supporting configurations such as a dual-socket
AMD Zen2, with 32 L3 monitoring domains and 256 RMIDs. To read both MBM
counters for all groups on all domains requires 32768 (32*256*2) counter
reads. The resolution of global, per-group MBM data which can be
provided is therefore sensitive to the cost of each counter read.
Furthermore, redirecting this much work to IPI handlers or worker
threads at a regular interval is disruptive to the present workload.

The test program fastcat, which was introduced in an earlier path, was
used to simulate the impact of this change on an optimized event
counter-reading procedure. The goal is to maximize the frequency at
which MBM counters can be dumped, so the benchmark determines the cost
of an additional global MBM counter sample.

The total number of cycles needed to read all local and total MBM
counters for a large number of monitoring groups was collected using the
perf tool. The test was run bound to a single CPU: once targeting
counters in the local domain and again for counters in a remote domain.
The cost of a dry-run reading no counters was substracted from the total
of each run to remove one-time setup costs.

AMD EPYC 7B12 64-Core Processor (250 mon groups)

Local Domain:   3.25M -> 1.22M (-62.5%)
Remote Domain:  7.91M -> 8.05M (+2.9%)

Intel(R) Xeon(R) Gold 6268CL CPU @ 2.80GHz (190 mon groups)

Local Domain:   2.98M -> 2.21M (-25.8%)
Remote Domain:  4.49M -> 4.62M (+3.1%)

Note that there is a small increase in overhead for remote domains,
which results from the introduction of a put_cpu() call to reenable
preemption after determining whether the fast path can be used. Users
sensitive to this cost should consider avoiding the remote counter read
penalty completely.

Also note that the Remote Domain results and the baseline Local Domain
results only measure cycles in the test program. Because all counter
reading work was carried out in kernel worker threads, the total system
cost of the operation is greater.

Fixes: 09909e098113 ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")

Signed-off-by: Peter Newman <peternewman@google.com>
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 200d89a640270..daaff1cfd3f24 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -541,6 +541,31 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 		return;
 	}
 
+	/*
+	 * If a performance-conscious caller has gone to the trouble of binding
+	 * their thread to the monitoring domain of the event counter, ensure
+	 * that the counters are read directly. smp_call_on_cpu()
+	 * unconditionally uses a work queue to read the counter, substantially
+	 * increasing the cost of the read.
+	 *
+	 * Preemption must be disabled to prevent a migration out of the domain
+	 * after the CPU is checked, which would result in reading the wrong
+	 * counters. Note that this makes the (slow) remote path a little slower
+	 * by requiring preemption to be reenabled when redirecting the request
+	 * to another domain was in fact necessary.
+	 *
+	 * In the case where all eligible target CPUs are nohz_full and
+	 * smp_call_function_any() is used, keep preemption disabled to avoid
+	 * the cost of reenabling it twice in the same read.
+	 */
+	cpu = get_cpu();
+	if (cpumask_test_cpu(cpu, cpumask)) {
+		mon_event_count(rr);
+		resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
+		put_cpu();
+		return;
+	}
+
 	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
 
 	/*
@@ -554,6 +579,9 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 	else
 		smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
 
+	/* If smp_call_function_any() was used, preemption is reenabled here. */
+	put_cpu();
+
 	resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
 }
 
-- 
2.47.0.199.ga7371fff76-goog

RE: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Luck, Tony]

>  Add a fast-path to ensure that requests bound to within the monitoring
> domain are read using a simple function call into mon_event_count()
> regardless of whether all CPUs in the target domain are using nohz_full.
...
> Fixes: 09909e098113 ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")
>
> Signed-off-by: Peter Newman <peternewman@google.com>

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Fenghua Yu]

Hi, Peter,

On 10/31/24 07:25, Peter Newman wrote:
> Performance-conscious users may use threads bound to CPUs within a
> specific monitoring domain to ensure that all bandwidth counters can be
> read efficiently. The hardware counters are only accessible to CPUs
> within the domain, so requests from CPUs outside the domain are
> forwarded to a kernel worker or IPI handler, incurring a substantial
> performance penalty on each read.  Recently, this penalty was observed
> to be paid by local reads as well.
> 
> To support blocking implementations of resctrl_arch_rmid_read(),
> mon_event_read() switched to smp_call_on_cpu() in most cases to read
> event counters using a kernel worker thread. Unlike
> smp_call_function_any(), which optimizes to a local function call when
> the calling CPU is in the target cpumask, smp_call_on_cpu() queues the
> work unconditionally.
> 
> Add a fast-path to ensure that requests bound to within the monitoring
> domain are read using a simple function call into mon_event_count()
> regardless of whether all CPUs in the target domain are using nohz_full.
> 
> This is significant when supporting configurations such as a dual-socket
> AMD Zen2, with 32 L3 monitoring domains and 256 RMIDs. To read both MBM
> counters for all groups on all domains requires 32768 (32*256*2) counter
> reads. The resolution of global, per-group MBM data which can be
> provided is therefore sensitive to the cost of each counter read.
> Furthermore, redirecting this much work to IPI handlers or worker
> threads at a regular interval is disruptive to the present workload.
> 
> The test program fastcat, which was introduced in an earlier path, was
> used to simulate the impact of this change on an optimized event
> counter-reading procedure. The goal is to maximize the frequency at
> which MBM counters can be dumped, so the benchmark determines the cost
> of an additional global MBM counter sample.
> 
> The total number of cycles needed to read all local and total MBM
> counters for a large number of monitoring groups was collected using the
> perf tool. The test was run bound to a single CPU: once targeting
> counters in the local domain and again for counters in a remote domain.
> The cost of a dry-run reading no counters was substracted from the total
> of each run to remove one-time setup costs.
> 
> AMD EPYC 7B12 64-Core Processor (250 mon groups)
> 
> Local Domain:   3.25M -> 1.22M (-62.5%)
> Remote Domain:  7.91M -> 8.05M (+2.9%)
> 
> Intel(R) Xeon(R) Gold 6268CL CPU @ 2.80GHz (190 mon groups)
> 
> Local Domain:   2.98M -> 2.21M (-25.8%)
> Remote Domain:  4.49M -> 4.62M (+3.1%)
> 
> Note that there is a small increase in overhead for remote domains,
> which results from the introduction of a put_cpu() call to reenable
> preemption after determining whether the fast path can be used. Users
> sensitive to this cost should consider avoiding the remote counter read
> penalty completely.
> 
> Also note that the Remote Domain results and the baseline Local Domain
> results only measure cycles in the test program. Because all counter
> reading work was carried out in kernel worker threads, the total system
> cost of the operation is greater.
> 
> Fixes: 09909e098113 ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 28 +++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 200d89a640270..daaff1cfd3f24 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -541,6 +541,31 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>   		return;
>   	}
>   
> +	/*
> +	 * If a performance-conscious caller has gone to the trouble of binding
> +	 * their thread to the monitoring domain of the event counter, ensure
> +	 * that the counters are read directly. smp_call_on_cpu()
> +	 * unconditionally uses a work queue to read the counter, substantially
> +	 * increasing the cost of the read.
> +	 *
> +	 * Preemption must be disabled to prevent a migration out of the domain
> +	 * after the CPU is checked, which would result in reading the wrong
> +	 * counters. Note that this makes the (slow) remote path a little slower
> +	 * by requiring preemption to be reenabled when redirecting the request
> +	 * to another domain was in fact necessary.
> +	 *
> +	 * In the case where all eligible target CPUs are nohz_full and
> +	 * smp_call_function_any() is used, keep preemption disabled to avoid
> +	 * the cost of reenabling it twice in the same read.
> +	 */
> +	cpu = get_cpu();
> +	if (cpumask_test_cpu(cpu, cpumask)) {
> +		mon_event_count(rr);
> +		resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
> +		put_cpu();
> +		return;
> +	}

This fast path code is a duplicate part of smp_call_funcion_any().

In nohz_full() case, the fast path doesn't gain much and even hurts 
remote domain performance:
1. On local domain, it may gain a little bit because it has a few lines 
less than directly calling smp_call_function_any(). But the gain is 
minor due to a lines less code, not due to heavy weight queued work.

2. On remote domain, it degrades performance because get_cpu() and 
put_cpu() are both called twice: one in the fast path code and one in 
smp_call_function_any(). As you mentioned earlier, put_cpu() impacts 
performance. I think get_cpu() has same impact too.

The fast path only gains in none nohz_full() case.

So maybe it's better to move the fast path code into the non nohz_full() 
case? With this change, you may have the following benefits:

1. No performance impact on nohz_full() case (either local or remote 
domain).
2. Improve performance on non nohz_full() case as you intended in this 
patch.
3. The fast path focuses on fixing the right performance bottleneck.

> +
>   	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
>   
>   	/*
> @@ -554,6 +579,9 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>   	else
>   		smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
>   
> +	/* If smp_call_function_any() was used, preemption is reenabled here. */
> +	put_cpu();
> +
>   	resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
>   }
>   

Thanks.

-Fenghua

RE: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Luck, Tony]

>   	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);

To a large degree Peter's is working around inefficiency in this housekeeping
call.

Code may be running on a suitable CPU from the domain cpumask, but this
call will very likely pick the first CPU in that mask, rather than the current one.

From that point it's all downhill unless you are lucky enough that the first
CPU is a tick_nohz_full_cpu() one and you take the

	smp_call_function_any(cpumask, mon_event_count, rr, 1);

path. It seems that on many systems you'll take the

	smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);

path and make a pointless IPI to get the data.

-Tony

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Fenghua Yu]

Hi, Tony,

On 11/4/24 14:56, Luck, Tony wrote:
>>    	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
> 
> To a large degree Peter's is working around inefficiency in this housekeeping
> call.
> 
> Code may be running on a suitable CPU from the domain cpumask, but this
> call will very likely pick the first CPU in that mask, rather than the current one.

Agree.

> 
>  From that point it's all downhill unless you are lucky enough that the first
> CPU is a tick_nohz_full_cpu() one and you take the
> 
> 	smp_call_function_any(cpumask, mon_event_count, rr, 1);

Whenever this function is called, the performance is degraded rather 
than improved because extra get_cpu()/put_cpu() are called in the fast 
path in the current patch.

On platforms that have less housekeeping CPUs (e.g. a RT platform), 
there could be a higher chance that the first CPU is a nohz_full CPU and 
run smp_call_function_any().

> 
> path. It seems that on many systems you'll take the
> 
> 	smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
> 
> path and make a pointless IPI to get the data.

Yes, that's right. But it's not conflicting with my suggested change.

What I suggested is to move the fast path code to this case only. So the 
fast path is always checked/called in both cases if condition is met:
1. It's already checked/called inside smp_call_function_any() in 
nohz_full case. No need to call out the fast path separately.
    - No extra get_cpu() and put_cpu() are called.
    - The performance is better than the current patch.
2. It's called out in non nohz_full case. No performance difference from 
the current patch.

Thanks.

-Fenghua

RE: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Luck, Tony]

> Whenever this function is called, the performance is degraded rather 
> than improved because extra get_cpu()/put_cpu() are called in the fast 
> path in the current patch.

But get_cpu()/put_cpu() aren't high overhead. Maybe costs less that the
cpumask_any_housekeeping() call that is avoided by Peter's patch.

Note that if Peter's patch doesn't take its fast path because the calling
CPU was on the wrong domain, then the subsequent code is going to
do an IPI whichever of the if/else path is taken.

-Tony

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Fenghua Yu]

Hi, Tony,

On 11/4/24 16:12, Luck, Tony wrote:
>> Whenever this function is called, the performance is degraded rather
>> than improved because extra get_cpu()/put_cpu() are called in the fast
>> path in the current patch.
> 
> But get_cpu()/put_cpu() aren't high overhead. Maybe costs less that the
> cpumask_any_housekeeping() call that is avoided by Peter's patch.

Quote from Peter:

"AMD EPYC 7B12 64-Core Processor (250 mon groups)

Local Domain:   3.25M -> 1.22M (-62.5%)
Remote Domain:  7.91M -> 8.05M (+2.9%)

Intel(R) Xeon(R) Gold 6268CL CPU @ 2.80GHz (190 mon groups)

Local Domain:   2.98M -> 2.21M (-25.8%)
Remote Domain:  4.49M -> 4.62M (+3.1%)

Note that there is a small increase in overhead for remote domains,
which results from the introduction of a put_cpu() call to reenable
preemption after determining whether the fast path can be used."

As his data shows, if the fast path is not taken, the extra put_cpu() 
itself costs +2.9% extra time on AMD machine and +3.1% extra time on 
Intel machine.

And this ~3% overhead is on top of queued work, which is more expensive 
than cpumask_any_housekeeping() IIUC.

> 
> Note that if Peter's patch doesn't take its fast path because the calling
> CPU was on the wrong domain, then the subsequent code is going to
> do an IPI whichever of the if/else path is taken.

In this case, actually IPI is only taken in smp_call_function_any() and 
smp_call_on_cpu() invokes a queued work instead of IPI.

My proposed change logically doesn't change Peter's fast path and 
performance for nohz_full/smp_call_on_cpu() case. It just utilizes the 
"built-in fast path already" inside smp_call_function_any() to save 
extra get_cpu() and put_cpu(). Hopefully the saved extra get_cpu() and 
put_cpu() can offset cost of cpumask_any_housekeeping().

 From Peter's commit message, seems nohz_full case is not 
called/measured a lot if any. If only one or a very few housekeeping 
CPUs on a large system, the nohz_full case will be called frequently and 
fast path will fail most of time and the extra get_cpu()/put_cpu() 
around the fast path might impact more on both local and total domain.

Thanks.

-Fenghua

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Peter Newman]

Hi Fenghua,

On Mon, Nov 4, 2024 at 11:36 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>
> Hi, Peter,
>
> On 10/31/24 07:25, Peter Newman wrote:

> > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > index 200d89a640270..daaff1cfd3f24 100644
> > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > @@ -541,6 +541,31 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> >               return;
> >       }
> >
> > +     /*
> > +      * If a performance-conscious caller has gone to the trouble of binding
> > +      * their thread to the monitoring domain of the event counter, ensure
> > +      * that the counters are read directly. smp_call_on_cpu()
> > +      * unconditionally uses a work queue to read the counter, substantially
> > +      * increasing the cost of the read.
> > +      *
> > +      * Preemption must be disabled to prevent a migration out of the domain
> > +      * after the CPU is checked, which would result in reading the wrong
> > +      * counters. Note that this makes the (slow) remote path a little slower
> > +      * by requiring preemption to be reenabled when redirecting the request
> > +      * to another domain was in fact necessary.
> > +      *
> > +      * In the case where all eligible target CPUs are nohz_full and
> > +      * smp_call_function_any() is used, keep preemption disabled to avoid
> > +      * the cost of reenabling it twice in the same read.
> > +      */
> > +     cpu = get_cpu();
> > +     if (cpumask_test_cpu(cpu, cpumask)) {
> > +             mon_event_count(rr);
> > +             resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
> > +             put_cpu();
> > +             return;
> > +     }
>
> This fast path code is a duplicate part of smp_call_funcion_any().
>
> In nohz_full() case, the fast path doesn't gain much and even hurts
> remote domain performance:
> 1. On local domain, it may gain a little bit because it has a few lines
> less than directly calling smp_call_function_any(). But the gain is
> minor due to a lines less code, not due to heavy weight queued work.
>
> 2. On remote domain, it degrades performance because get_cpu() and
> put_cpu() are both called twice: one in the fast path code and one in
> smp_call_function_any(). As you mentioned earlier, put_cpu() impacts
> performance. I think get_cpu() has same impact too.

get_cpu() and put_cpu() nest, so only the put_cpu() that reduces the
preempt count to 0 will call into the scheduler. See the source
comment I had added below.

But... note that below smp_call_on_cpu() is now called with preemption
disabled. (Looks like I only benchmarked and never ran a debug
build...) I will have to change the patch to make sure put_cpu() is
called before smp_call_on_cpu().


>
> The fast path only gains in none nohz_full() case.
>
> So maybe it's better to move the fast path code into the non nohz_full()
> case? With this change, you may have the following benefits:
>
> 1. No performance impact on nohz_full() case (either local or remote
> domain).
> 2. Improve performance on non nohz_full() case as you intended in this
> patch.
> 3. The fast path focuses on fixing the right performance bottleneck.

The consequence of reusing the current-cpu-in-mask check in
smp_call_function_any() is that if the check fails, it could cause
resctrl_arch_rmid_read() to fail by invoking it in an IPI handler when
it would have succeeded if invoked on a kernel worker, undoing James's
original work.

-Peter


>
> > +
> >       cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
> >
> >       /*
> > @@ -554,6 +579,9 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> >       else
> >               smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
> >
> > +     /* If smp_call_function_any() was used, preemption is reenabled here. */
> > +     put_cpu();
> > +
> >       resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
> >   }
> >
>
> Thanks.
>
> -Fenghua

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Reinette Chatre]

Hi Peter,

On 11/5/24 3:25 AM, Peter Newman wrote:
> Hi Fenghua,
> 
> On Mon, Nov 4, 2024 at 11:36 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>>
>> Hi, Peter,
>>
>> On 10/31/24 07:25, Peter Newman wrote:
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> index 200d89a640270..daaff1cfd3f24 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> @@ -541,6 +541,31 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>>>               return;
>>>       }
>>>
>>> +     /*
>>> +      * If a performance-conscious caller has gone to the trouble of binding
>>> +      * their thread to the monitoring domain of the event counter, ensure
>>> +      * that the counters are read directly. smp_call_on_cpu()
>>> +      * unconditionally uses a work queue to read the counter, substantially
>>> +      * increasing the cost of the read.
>>> +      *
>>> +      * Preemption must be disabled to prevent a migration out of the domain
>>> +      * after the CPU is checked, which would result in reading the wrong
>>> +      * counters. Note that this makes the (slow) remote path a little slower
>>> +      * by requiring preemption to be reenabled when redirecting the request
>>> +      * to another domain was in fact necessary.
>>> +      *
>>> +      * In the case where all eligible target CPUs are nohz_full and
>>> +      * smp_call_function_any() is used, keep preemption disabled to avoid
>>> +      * the cost of reenabling it twice in the same read.
>>> +      */
>>> +     cpu = get_cpu();
>>> +     if (cpumask_test_cpu(cpu, cpumask)) {
>>> +             mon_event_count(rr);
>>> +             resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
>>> +             put_cpu();
>>> +             return;
>>> +     }
>>
>> This fast path code is a duplicate part of smp_call_funcion_any().
>>
>> In nohz_full() case, the fast path doesn't gain much and even hurts
>> remote domain performance:
>> 1. On local domain, it may gain a little bit because it has a few lines
>> less than directly calling smp_call_function_any(). But the gain is
>> minor due to a lines less code, not due to heavy weight queued work.
>>
>> 2. On remote domain, it degrades performance because get_cpu() and
>> put_cpu() are both called twice: one in the fast path code and one in
>> smp_call_function_any(). As you mentioned earlier, put_cpu() impacts
>> performance. I think get_cpu() has same impact too.
> 
> get_cpu() and put_cpu() nest, so only the put_cpu() that reduces the
> preempt count to 0 will call into the scheduler. See the source
> comment I had added below.
> 
> But... note that below smp_call_on_cpu() is now called with preemption
> disabled. (Looks like I only benchmarked and never ran a debug
> build...) I will have to change the patch to make sure put_cpu() is
> called before smp_call_on_cpu().
> 
> 
>>
>> The fast path only gains in none nohz_full() case.
>>
>> So maybe it's better to move the fast path code into the non nohz_full()
>> case? With this change, you may have the following benefits:
>>
>> 1. No performance impact on nohz_full() case (either local or remote
>> domain).
>> 2. Improve performance on non nohz_full() case as you intended in this
>> patch.
>> 3. The fast path focuses on fixing the right performance bottleneck.
> 
> The consequence of reusing the current-cpu-in-mask check in
> smp_call_function_any() is that if the check fails, it could cause
> resctrl_arch_rmid_read() to fail by invoking it in an IPI handler when
> it would have succeeded if invoked on a kernel worker, undoing James's
> original work.

I think this change already undoes the motivation for 09909e098113
("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
This change will call resctrl_arch_rmid_read() with preemption disabled if
it happens to be called on CPU in monitoring domain. Would that not cause
MPAM monitor count reads from CPU in domain to be a bug?

Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?

Reinette

RE: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Luck, Tony]

> I think this change already undoes the motivation for 09909e098113
> ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
> changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
> This change will call resctrl_arch_rmid_read() with preemption disabled if
> it happens to be called on CPU in monitoring domain. Would that not cause
> MPAM monitor count reads from CPU in domain to be a bug?
>
> Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?

How is this all going to look after the split into fs/resctrl and arch/* ?

Is the file system code going to have implementation choices that prevent
performance sensitive users like Peter from optimizing monitor event
reads by binding the monitor process to a CPU in the right domain
to avoid IPI?

-Tony

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Reinette Chatre]

Hi Tony,

On 11/5/24 3:39 PM, Luck, Tony wrote:
>> I think this change already undoes the motivation for 09909e098113
>> ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
>> changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
>> This change will call resctrl_arch_rmid_read() with preemption disabled if
>> it happens to be called on CPU in monitoring domain. Would that not cause
>> MPAM monitor count reads from CPU in domain to be a bug?
>>
>> Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?
> 
> How is this all going to look after the split into fs/resctrl and arch/* ?

Unclear to me at this point. Peter exposed an issue with current implementation
and this needs to be fixed. Since this involves preparatory work that impacts
systems currently supported we could also consider reverting to original behavior
and go back to drawing board with the preparatory work.

> Is the file system code going to have implementation choices that prevent
> performance sensitive users like Peter from optimizing monitor event
> reads by binding the monitor process to a CPU in the right domain
> to avoid IPI?

Apologies for not clearly stating it but I do agree that there is an issue
that needs to be fixed.

My response was not intended to be interpreted as a NACK but instead an attempt
to engage in discussion by pointing out that the proposed fix may not be ideal.

I tried out my own suggestion and indeed when just trying to mount resctrl
on x86 with this patch applied results in:
	BUG: scheduling while atomic

I do not object to optimizing monitor event reads but the proposed fix
is not appropriate in its current form.

Reinette

 

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Peter Newman]

Hi Reinette,

On Wed, Nov 6, 2024 at 1:13 AM Reinette Chatre
<reinette.chatre@intel.com> wrote:
>
> Hi Tony,
>
> On 11/5/24 3:39 PM, Luck, Tony wrote:
> >> I think this change already undoes the motivation for 09909e098113
> >> ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
> >> changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
> >> This change will call resctrl_arch_rmid_read() with preemption disabled if
> >> it happens to be called on CPU in monitoring domain. Would that not cause
> >> MPAM monitor count reads from CPU in domain to be a bug?
> >>
> >> Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?
> >
> > How is this all going to look after the split into fs/resctrl and arch/* ?
>
> Unclear to me at this point. Peter exposed an issue with current implementation
> and this needs to be fixed. Since this involves preparatory work that impacts
> systems currently supported we could also consider reverting to original behavior
> and go back to drawing board with the preparatory work.
>
> > Is the file system code going to have implementation choices that prevent
> > performance sensitive users like Peter from optimizing monitor event
> > reads by binding the monitor process to a CPU in the right domain
> > to avoid IPI?
>
> Apologies for not clearly stating it but I do agree that there is an issue
> that needs to be fixed.
>
> My response was not intended to be interpreted as a NACK but instead an attempt
> to engage in discussion by pointing out that the proposed fix may not be ideal.
>
> I tried out my own suggestion and indeed when just trying to mount resctrl
> on x86 with this patch applied results in:
>         BUG: scheduling while atomic
>
> I do not object to optimizing monitor event reads but the proposed fix
> is not appropriate in its current form.

Yes, I mentioned the atomic sleep issues in my reply to Fenghua. I
expect even the new case I added will have problems on
resctrl_arch_rmid_read() implementations which block, since it would
need to be called with preemption disabled to ensure invocation by a
direct function call stays in the right domain. The last proposed MPAM
resctrl_arch_rmid_read() implementation I saw can return an error when
called in an atomic context, so that means my change would cause slow
counters (i.e., MPAM CSU) to be unreadable when read from the local
domain. (Or totally unreadable on a single-domain machine.)

As a refresher, the original issue that led to this situation was how
an MPAM CSU (cache occupancy) monitor can be installed in response to
a read request. The number of monitors is usually small (or just 1),
so they need to be frequently installed, there can be access issues
depending on what CPU wants to read which domain, and installing a
monitor is a slow operation that requires waiting.

https://lore.kernel.org/lkml/670081d0-b4fc-79c5-68f8-5b3c162b74b9@arm.com/

Thanks,
-Peter

Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Peter Newman]

Hi Fenghua,

On Tue, Nov 5, 2024 at 12:25 PM Peter Newman <peternewman@google.com> wrote:
>
> Hi Fenghua,
>
> On Mon, Nov 4, 2024 at 11:36 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
> >
> > Hi, Peter,
> >
> > On 10/31/24 07:25, Peter Newman wrote:
>
> > > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > index 200d89a640270..daaff1cfd3f24 100644
> > > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > @@ -541,6 +541,31 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> > >               return;
> > >       }
> > >
> > > +     /*
> > > +      * If a performance-conscious caller has gone to the trouble of binding
> > > +      * their thread to the monitoring domain of the event counter, ensure
> > > +      * that the counters are read directly. smp_call_on_cpu()
> > > +      * unconditionally uses a work queue to read the counter, substantially
> > > +      * increasing the cost of the read.
> > > +      *
> > > +      * Preemption must be disabled to prevent a migration out of the domain
> > > +      * after the CPU is checked, which would result in reading the wrong
> > > +      * counters. Note that this makes the (slow) remote path a little slower
> > > +      * by requiring preemption to be reenabled when redirecting the request
> > > +      * to another domain was in fact necessary.
> > > +      *
> > > +      * In the case where all eligible target CPUs are nohz_full and
> > > +      * smp_call_function_any() is used, keep preemption disabled to avoid
> > > +      * the cost of reenabling it twice in the same read.
> > > +      */
> > > +     cpu = get_cpu();
> > > +     if (cpumask_test_cpu(cpu, cpumask)) {
> > > +             mon_event_count(rr);
> > > +             resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
> > > +             put_cpu();
> > > +             return;
> > > +     }
> >
> > This fast path code is a duplicate part of smp_call_funcion_any().
> >
> > In nohz_full() case, the fast path doesn't gain much and even hurts
> > remote domain performance:
> > 1. On local domain, it may gain a little bit because it has a few lines
> > less than directly calling smp_call_function_any(). But the gain is
> > minor due to a lines less code, not due to heavy weight queued work.
> >
> > 2. On remote domain, it degrades performance because get_cpu() and
> > put_cpu() are both called twice: one in the fast path code and one in
> > smp_call_function_any(). As you mentioned earlier, put_cpu() impacts
> > performance. I think get_cpu() has same impact too.
>
> get_cpu() and put_cpu() nest, so only the put_cpu() that reduces the
> preempt count to 0 will call into the scheduler. See the source
> comment I had added below.
>
> But... note that below smp_call_on_cpu() is now called with preemption
> disabled. (Looks like I only benchmarked and never ran a debug
> build...) I will have to change the patch to make sure put_cpu() is
> called before smp_call_on_cpu().
>
>
> >
> > The fast path only gains in none nohz_full() case.
> >
> > So maybe it's better to move the fast path code into the non nohz_full()
> > case? With this change, you may have the following benefits:
> >
> > 1. No performance impact on nohz_full() case (either local or remote
> > domain).
> > 2. Improve performance on non nohz_full() case as you intended in this
> > patch.
> > 3. The fast path focuses on fixing the right performance bottleneck.
>
> The consequence of reusing the current-cpu-in-mask check in
> smp_call_function_any() is that if the check fails, it could cause
> resctrl_arch_rmid_read() to fail by invoking it in an IPI handler when
> it would have succeeded if invoked on a kernel worker, undoing James's
> original work.

I noted in my earlier email to Reinette that this is already a problem
in the fast path case I added.

I ended up creating a resctrl_arch_event_read_blocks() hook for asking
the implementation whether a particular event actually blocks so that
the workqueue is only used when it's really necessary:

https://lore.kernel.org/lkml/20241106154306.2721688-2-peternewman@google.com/

With this, I was able to follow your suggestion of using
smp_call_function_any() for the local counter read.

However, it's worth noting that this ended up making remote reads
faster on AMD but slower on Intel for some reason. I assume it's a
flaw in my benchmark where the IPIs were cross-socket on Intel while
they were only cross-chiplet on AMD.

Thanks!
-Peter

RE: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Luck, Tony]

> As a refresher, the original issue that led to this situation was how
> an MPAM CSU (cache occupancy) monitor can be installed in response to
> a read request. The number of monitors is usually small (or just 1),
> so they need to be frequently installed, there can be access issues
> depending on what CPU wants to read which domain, and installing a
> monitor is a slow operation that requires waiting.

Maybe the ARM implementation should adopt a similar approach to
Babu's ABMC patches and provide an explicit mechanism to bind a
h/w counter to an instance of a monitor event. Binding on demand
when a user reads an event file seems awkward for users. Especially
of the number of counters is >1, but unknown.

-Tony