[bpf-next,v7,4/4] selftests/bpf: add usage example for cpu cycles kfuncs

Commit Message

Vadim Fedorenko Nov. 18, 2024, 6:52 p.m. UTC

The selftest provides an example of how to measure the latency of bpf
kfunc/helper call in cycles and nanoseconds.

Signed-off-by: Vadim Fedorenko <[e-mail]>
---
 .../bpf/prog_tests/test_cpu_cycles.c          | 35 +++++++++++++++++++
 .../selftests/bpf/progs/test_cpu_cycles.c     | 25 +++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_cpu_cycles.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_cpu_cycles.c

Comments

Peter Zijlstra Nov. 19, 2024, 11:47 a.m. UTC | #1

On Mon, Nov 18, 2024 at 10:52:45AM -0800, Vadim Fedorenko wrote:

> +int bpf_cpu_cycles(void)
> +{
> +	struct bpf_pidns_info pidns;
> +	__u64 start;
> +
> +	start = bpf_get_cpu_cycles();
> +	bpf_get_ns_current_pid_tgid(0, 0, &pidns, sizeof(struct bpf_pidns_info));
> +	cycles = bpf_get_cpu_cycles() - start;
> +	ns = bpf_cpu_cycles_to_ns(cycles);
> +	return 0;
> +}

Oh, the intent is to use that cycles_to_ns() on deltas. That wasn't at
all clear.

Anyway, the above has more problems than just bad naming. TSC is
constant and not affected by DVFS, so depending on the DVFS state of
things your function will return wildly different readings.

Why do you think you need this?

Vadim Fedorenko Nov. 19, 2024, 2:45 p.m. UTC | #2

On 19/11/2024 03:47, Peter Zijlstra wrote:
> On Mon, Nov 18, 2024 at 10:52:45AM -0800, Vadim Fedorenko wrote:
> 
>> +int bpf_cpu_cycles(void)
>> +{
>> +	struct bpf_pidns_info pidns;
>> +	__u64 start;
>> +
>> +	start = bpf_get_cpu_cycles();
>> +	bpf_get_ns_current_pid_tgid(0, 0, &pidns, sizeof(struct bpf_pidns_info));
>> +	cycles = bpf_get_cpu_cycles() - start;
>> +	ns = bpf_cpu_cycles_to_ns(cycles);
>> +	return 0;
>> +}
> 
> Oh, the intent is to use that cycles_to_ns() on deltas. That wasn't at
> all clear.

Yep, that's the main use case, it was discussed in the previous
versions of the patchset.

> 
> Anyway, the above has more problems than just bad naming. TSC is
> constant and not affected by DVFS, so depending on the DVFS state of
> things your function will return wildly different readings.

Why should I care about DVFS? The use case is to measure the time spent
in some code. If we replace it with bpf_ktime_get_ns(), it will also be
affected by DVFS, and it's fine. We will be able to see the difference
for different DVFS states.

> Why do you think you need this?

Peter Zijlstra Nov. 20, 2024, 8:51 a.m. UTC | #3

On Tue, Nov 19, 2024 at 06:45:57AM -0800, Vadim Fedorenko wrote:
> On 19/11/2024 03:47, Peter Zijlstra wrote:
> > On Mon, Nov 18, 2024 at 10:52:45AM -0800, Vadim Fedorenko wrote:
> > 
> > > +int bpf_cpu_cycles(void)
> > > +{
> > > +	struct bpf_pidns_info pidns;
> > > +	__u64 start;
> > > +
> > > +	start = bpf_get_cpu_cycles();
> > > +	bpf_get_ns_current_pid_tgid(0, 0, &pidns, sizeof(struct bpf_pidns_info));
> > > +	cycles = bpf_get_cpu_cycles() - start;
> > > +	ns = bpf_cpu_cycles_to_ns(cycles);
> > > +	return 0;
> > > +}
> > 
> > Oh, the intent is to use that cycles_to_ns() on deltas. That wasn't at
> > all clear.
> 
> Yep, that's the main use case, it was discussed in the previous
> versions of the patchset.

Should bloody well be in the changelogs then. As is I'm tempted to NAK
the entire series because there is not a single word on WHY for any of
this.

> > Anyway, the above has more problems than just bad naming. TSC is
> > constant and not affected by DVFS, so depending on the DVFS state of
> > things your function will return wildly different readings.
> 
> Why should I care about DVFS? The use case is to measure the time spent
> in some code. If we replace it with bpf_ktime_get_ns(), it will also be
> affected by DVFS, and it's fine. We will be able to see the difference
> for different DVFS states.

Again, this goes to usage, why do you want this, what are you going to
do with the results?

Run-ro-run numbers will be absolutely useless because of DVFS.

Vadim Fedorenko Nov. 20, 2024, 5:19 p.m. UTC | #4

On 20/11/2024 00:51, Peter Zijlstra wrote:
> On Tue, Nov 19, 2024 at 06:45:57AM -0800, Vadim Fedorenko wrote:
>> On 19/11/2024 03:47, Peter Zijlstra wrote:
>>> On Mon, Nov 18, 2024 at 10:52:45AM -0800, Vadim Fedorenko wrote:
>>>
>>>> +int bpf_cpu_cycles(void)
>>>> +{
>>>> +	struct bpf_pidns_info pidns;
>>>> +	__u64 start;
>>>> +
>>>> +	start = bpf_get_cpu_cycles();
>>>> +	bpf_get_ns_current_pid_tgid(0, 0, &pidns, sizeof(struct bpf_pidns_info));
>>>> +	cycles = bpf_get_cpu_cycles() - start;
>>>> +	ns = bpf_cpu_cycles_to_ns(cycles);
>>>> +	return 0;
>>>> +}
>>>
>>> Oh, the intent is to use that cycles_to_ns() on deltas. That wasn't at
>>> all clear.
>>
>> Yep, that's the main use case, it was discussed in the previous
>> versions of the patchset.
> 
> Should bloody well be in the changelogs then. As is I'm tempted to NAK
> the entire series because there is not a single word on WHY for any of
> this.

Sure, I'll add this info in the next version.

>>> Anyway, the above has more problems than just bad naming. TSC is
>>> constant and not affected by DVFS, so depending on the DVFS state of
>>> things your function will return wildly different readings.
>>
>> Why should I care about DVFS? The use case is to measure the time spent
>> in some code. If we replace it with bpf_ktime_get_ns(), it will also be
>> affected by DVFS, and it's fine. We will be able to see the difference
>> for different DVFS states.
> 
> Again, this goes to usage, why do you want this, what are you going to
> do with the results?
> 
> Run-ro-run numbers will be absolutely useless because of DVFS.

We do a lot of measurements of bpf programs and kernel stack functions
at scale. We can filter out variations due to DVFS as well as slice the
results by the HW generations, etc. In general, we do see benefits of
the values we gather. The goal of this patchset is to optimize the
overhead added by bpf_ktime_get_ns(). Andrii has already shown the
benefit in [1]. TL;DR - it's about 35-40% faster than the pair of
bpf_ktime_get_ns(). And helps to save a lot of CPU at scale.

[1] 
https://lore.kernel.org/bpf/CAEf4BzaRb+fUK17wrj4sWnYM5oKxTvwZC=[e-mail]/

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/test_cpu_cycles.c b/tools/testing/selftests/bpf/prog_tests/test_cpu_cycles.c
new file mode 100644
index 000000000000..d7f3b66594b3
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_cpu_cycles.c
@@ -0,0 +1,35 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Inc. */
+
+#include <test_progs.h>
+#include "test_cpu_cycles.skel.h"
+
+static void cpu_cycles(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	struct test_cpu_cycles *skel;
+	int err, pfd;
+
+	skel = test_cpu_cycles__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_cpu_cycles open and load"))
+		return;
+
+	pfd = bpf_program__fd(skel->progs.bpf_cpu_cycles);
+	if (!ASSERT_GT(pfd, 0, "test_cpu_cycles fd"))
+		goto fail;
+
+	err = bpf_prog_test_run_opts(pfd, &opts);
+	if (!ASSERT_OK(err, "test_cpu_cycles test run"))
+		goto fail;
+
+	ASSERT_NEQ(skel->bss->cycles, 0, "test_cpu_cycles 0 cycles");
+	ASSERT_NEQ(skel->bss->ns, 0, "test_cpu_cycles 0 ns");
+fail:
+	test_cpu_cycles__destroy(skel);
+}
+
+void test_cpu_cycles(void)
+{
+	if (test__start_subtest("cpu_cycles"))
+		cpu_cycles();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_cpu_cycles.c b/tools/testing/selftests/bpf/progs/test_cpu_cycles.c
new file mode 100644
index 000000000000..48762f07ad7f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_cpu_cycles.c
@@ -0,0 +1,25 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Inc. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+extern u64 bpf_cpu_cycles_to_ns(u64 cycles) __weak __ksym;
+extern u64 bpf_get_cpu_cycles(void) __weak __ksym;
+
+__u64 cycles, ns;
+
+SEC("syscall")
+int bpf_cpu_cycles(void)
+{
+	struct bpf_pidns_info pidns;
+	__u64 start;
+
+	start = bpf_get_cpu_cycles();
+	bpf_get_ns_current_pid_tgid(0, 0, &pidns, sizeof(struct bpf_pidns_info));
+	cycles = bpf_get_cpu_cycles() - start;
+	ns = bpf_cpu_cycles_to_ns(cycles);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";