Re: [PATCH net-next v3] page_pool: import Jesper's page_pool benchmark

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 13 Jun 2025 23:44:20 +0000 Mina Almasry wrote:
> From: Jesper Dangaard Brouer <hawk@kernel.org>
> 
> We frequently consult with Jesper's out-of-tree page_pool benchmark to
> evaluate page_pool changes.
> 
> Import the benchmark into the upstream linux kernel tree so that (a)
> we're all running the same version, (b) pave the way for shared
> improvements, and (c) maybe one day integrate it with nipa, if possible.
> 
> Import bench_page_pool_simple from commit 35b1716d0c30 ("Add
> page_bench06_walk_all"), from this repository:
> https://github.com/netoptimizer/prototype-kernel.git
> 
> Changes done during upstreaming:
> - Fix checkpatch issues.

There is more:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#81: 
new file mode 100644

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#122: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:1:
+/*

CHECK: Please use a blank line after function/struct/union/enum declarations
#163: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:42:
+};
+#define bit(b)		(1 << (b))

WARNING: Block comments use a trailing */ on a separate line
#172: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:51:
+ * introduced by the for loop itself */

WARNING: Prefer kcalloc over kzalloc with multiply
#238: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:117:
+	array = kzalloc(sizeof(struct page *) * elems, gfp_mask);

WARNING: braces {} are not necessary for single statement blocks
#240: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:119:
+	for (i = 0; i < elems; i++) {
+		array[i] = page_pool_alloc_pages(pp, gfp_mask);
+	}

WARNING: braces {} are not necessary for single statement blocks
#243: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:122:
+	for (i = 0; i < elems; i++) {
+		_page_pool_put_page(pp, array[i], false);
+	}

WARNING: line length of 81 exceeds 80 columns
#288: FILE: tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:167:
+		/* Common fast-path alloc, that depend on in_serving_softirq() */

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#402: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:1:
+/*

WARNING: line length of 81 exceeds 80 columns
#451: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:50:
+ * Architectures Software Developer’s Manual Volume 3: System Programming Guide

CHECK: Alignment should match open parenthesis
#491: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:90:
+		perf_event = perf_event_create_kernel_counter(&perf_conf, cpu,
+						 NULL /* task */,

CHECK: spaces preferred around that '|' (ctx:VxV)
#626: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:225:
+	rec.flags       = (TIME_BENCH_LOOP|TIME_BENCH_TSC|TIME_BENCH_WALLCLOCK);
 	                                  ^

CHECK: spaces preferred around that '|' (ctx:VxV)
#626: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:225:
+	rec.flags       = (TIME_BENCH_LOOP|TIME_BENCH_TSC|TIME_BENCH_WALLCLOCK);
 	                                                 ^

CHECK: Lines should not end with a '('
#743: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:342:
+void time_bench_run_concurrent(

CHECK: spaces preferred around that '|' (ctx:VxV)
#772: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:371:
+		c->rec.flags       = (TIME_BENCH_LOOP|TIME_BENCH_TSC|
 		                                     ^

CHECK: space preferred before that '|' (ctx:VxE)
#772: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:371:
+		c->rec.flags       = (TIME_BENCH_LOOP|TIME_BENCH_TSC|
 		                                                    ^

WARNING: Missing a blank line after declarations
#801: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.c:400:
+		struct time_bench_cpu *c = &cpu_tasks[cpu];
+		kthread_stop(c->task);

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#814: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:1:
+/*

WARNING: please, no space before tabs
#829: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:16:
+^Iuint32_t flags; ^I/* Measurements types enabled */$

CHECK: spaces preferred around that '<<' (ctx:VxV)
#830: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:17:
+#define TIME_BENCH_LOOP		(1<<0)
                        		  ^

CHECK: Prefer using the BIT macro
#830: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:17:
+#define TIME_BENCH_LOOP		(1<<0)

CHECK: spaces preferred around that '<<' (ctx:VxV)
#831: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:18:
+#define TIME_BENCH_TSC		(1<<1)
                       		  ^

CHECK: Prefer using the BIT macro
#831: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:18:
+#define TIME_BENCH_TSC		(1<<1)

CHECK: spaces preferred around that '<<' (ctx:VxV)
#832: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:19:
+#define TIME_BENCH_WALLCLOCK	(1<<2)
                             	  ^

CHECK: Prefer using the BIT macro
#832: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:19:
+#define TIME_BENCH_WALLCLOCK	(1<<2)

CHECK: spaces preferred around that '<<' (ctx:VxV)
#833: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:20:
+#define TIME_BENCH_PMU		(1<<3)
                       		  ^

CHECK: Prefer using the BIT macro
#833: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:20:
+#define TIME_BENCH_PMU		(1<<3)

WARNING: please, no space before tabs
#838: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:25:
+^Iuint64_t invoked_cnt; ^I/* Returned actual invocations */$

WARNING: Block comments use a trailing */ on a separate line
#844: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:31:
+	 * instructions counter including pipelined instructions */

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#917: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:104:
+	unsigned hi, lo;

WARNING: Missing a blank line after declarations
#918: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:105:
+	unsigned hi, lo;
+	asm volatile("CPUID\n\t"

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#930: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:117:
+	unsigned hi, lo;

WARNING: Missing a blank line after declarations
#931: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:118:
+	unsigned hi, lo;
+	asm volatile("RDTSCP\n\t"

WARNING: Block comments use * on subsequent lines
#955: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:142:
+/*
+inline uint64_t rdtsc(void)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#997: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:184:
+static __always_inline unsigned long long p_rdpmc(unsigned in)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#999: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:186:
+	unsigned d, a;

CHECK: Lines should not end with a '('
#1038: FILE: tools/testing/selftests/net/bench/page_pool/time_bench.h:225:
+void time_bench_run_concurrent(

WARNING: line length of 113 exceeds 80 columns
#1097: FILE: tools/testing/selftests/net/bench/test_bench_page_pool.sh:19:
+	echo ${result} | grep -o -E "no-softirq-page_pool01 Per elem: ([0-9]+) cycles\(tsc\) ([0-9]+\.[0-9]+) ns"

WARNING: line length of 113 exceeds 80 columns
#1101: FILE: tools/testing/selftests/net/bench/test_bench_page_pool.sh:23:
+	echo ${result} | grep -o -E "no-softirq-page_pool02 Per elem: ([0-9]+) cycles\(tsc\) ([0-9]+\.[0-9]+) ns"

WARNING: line length of 113 exceeds 80 columns
#1105: FILE: tools/testing/selftests/net/bench/test_bench_page_pool.sh:27:
+	echo ${result} | grep -o -E "no-softirq-page_pool03 Per elem: ([0-9]+) cycles\(tsc\) ([0-9]+\.[0-9]+) ns"

total: 0 errors, 33 warnings, 17 checks, 995 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit 12b4bc1bc38a ("page_pool: import Jesper's page_pool benchmark") has style problems, please review.

NOTE: Ignored message types: ALLOC_SIZEOF_STRUCT BAD_REPORTED_BY_LINK CAMELCASE COMMIT_LOG_LONG_LINE GIT_COMMIT_ID MACRO_ARG_REUSE NO_AUTHOR_SIGN_OFF

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.
total: 0 errors, 33 warnings, 17 checks, 995 lines checked

> diff --git a/tools/testing/selftests/net/bench/page_pool/time_bench.c b/tools/testing/selftests/net/bench/page_pool/time_bench.c
> new file mode 100644
> index 000000000000..257b1515c64e
> --- /dev/null
> +++ b/tools/testing/selftests/net/bench/page_pool/time_bench.c
> @@ -0,0 +1,406 @@
> +/*
> + * Benchmarking code execution time inside the kernel
> + *
> + * Copyright (C) 2014, Red Hat, Inc., Jesper Dangaard Brouer
> + *  for licensing details see kernel-base/COPYING

don't think kernel-base/COPYING exists

coccicheck also says:

testing/tools/testing/selftests/net/bench/page_pool/time_bench.c:57:36-37: WARNING: Use ARRAY_SIZE
testing/tools/testing/selftests/net/bench/page_pool/bench_page_pool_simple.c:223:5-17: Unneeded variable: "passed_count". Return "0" on line 245

IIUC the former is in user space code, but maybe we can add a define
for ARRAY_SIZE and use it? It's pretty trivial.
-- 
pw-bot: cr