[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250614100853.3f2372f2@kernel.org>
Date: Sat, 14 Jun 2025 10:08:53 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, Jesper Dangaard Brouer <hawk@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
<horms@...nel.org>, Shuah Khan <shuah@...nel.org>, Ilias Apalodimas
<ilias.apalodimas@...aro.org>, "Toke Høiland-Jørgensen" <toke@...e.dk>
Subject: Re: [PATCH net-next v3] page_pool: import Jesper's page_pool
benchmark
On Fri, 13 Jun 2025 23:44:20 +0000 Mina Almasry wrote:
> From: Jesper Dangaard Brouer <hawk@...nel.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
Powered by blists - more mailing lists