[PATCH bpf-next 0/5] introduce bpf_strncmp() helper

[PATCH bpf-next 0/5] introduce bpf_strncmp() helper

[Hou Tao]

Hi,

The motivation for introducing bpf_strncmp() helper comes from
two aspects:

(1) clang doesn't always replace strncmp() automatically
In tracing program, sometimes we need to using a home-made
strncmp() to check whether or not the file name is expected.

(2) the performance of home-made strncmp is not so good
As shown in the benchmark in patch #4, the performance of
bpf_strncmp() helper is 18% or 33% better than home-made strncmp()
under x86-64 or arm64 when the compared string length is 64. When
the string length grows to 4095, the performance win will be
179% or 600% under x86-64 or arm64.

The prototype of bpf_strncmp() has changed from

  bpf_strncmp(const char *s1, const char *s2, u32 s2_sz)

to

  bpf_strncmp(const char *s1, u32 s1_sz, const char *s2)

The main reason is readability and there is nearly no performance
difference between these two APIs (refer to the data attached below
[1]).

Any comments are welcome.
Regards,
Tao

Change Log:
v1:
 * change API to bpf_strncmp(const char *s1, u32 s1_sz, const char *s2)
 * add benchmark refactor and benchmark between bpf_strncmp() and strncmp()

RFC: https://lore.kernel.org/bpf/20211106132822.1396621-1-houtao1@huawei.com/

[1] Performance difference between two APIs under x86-64:

helper_rfc-X: use bpf_strncmp in RFC to compare X-sized string
helper-Y: use bpf_strncmp in v1 to compare Y-sized string

helper_rfc-1         3.482 ± 0.002M/s (drops 0.000 ± 0.000M/s)
helper-1             3.485 ± 0.001M/s (drops 0.000 ± 0.000M/s)

helper_rfc-8         3.428 ± 0.001M/s (drops 0.000 ± 0.000M/s)
helper-8             3.434 ± 0.001M/s (drops 0.000 ± 0.000M/s)

helper_rfc-32        3.253 ± 0.002M/s (drops 0.000 ± 0.000M/s)
helper-32            3.234 ± 0.001M/s (drops 0.000 ± 0.000M/s)

helper_rfc-64        3.039 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-64            3.042 ± 0.001M/s (drops 0.000 ± 0.000M/s)

helper_rfc-128       2.640 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-128           2.633 ± 0.000M/s (drops 0.000 ± 0.000M/s)

helper_rfc-512       1.576 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-512           1.574 ± 0.000M/s (drops 0.000 ± 0.000M/s)

helper_rfc-2048      0.602 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-2048          0.602 ± 0.000M/s (drops 0.000 ± 0.000M/s)

helper_rfc-4095      0.328 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-4095          0.328 ± 0.000M/s (drops 0.000 ± 0.000M/s)

Hou Tao (5):
  bpf: add bpf_strncmp helper
  selftests/bpf: fix checkpatch error on empty function parameter
  selftests/bpf: factor out common helpers for benchmarks
  selftests/bpf: add benchmark for bpf_strncmp() helper
  selftests/bpf: add test cases for bpf_strncmp()

 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |  11 ++
 kernel/bpf/helpers.c                          |  16 ++
 tools/include/uapi/linux/bpf.h                |  11 ++
 tools/testing/selftests/bpf/Makefile          |   4 +-
 tools/testing/selftests/bpf/bench.c           |  21 ++-
 tools/testing/selftests/bpf/bench.h           |  34 +++-
 .../bpf/benchs/bench_bloom_filter_map.c       |  44 ++---
 .../selftests/bpf/benchs/bench_count.c        |  16 +-
 .../selftests/bpf/benchs/bench_rename.c       |  43 ++---
 .../selftests/bpf/benchs/bench_ringbufs.c     |  21 +--
 .../selftests/bpf/benchs/bench_strncmp.c      | 150 ++++++++++++++++
 .../selftests/bpf/benchs/bench_trigger.c      |  79 ++++----
 .../selftests/bpf/benchs/run_bench_strncmp.sh |  12 ++
 .../selftests/bpf/prog_tests/test_strncmp.c   | 170 ++++++++++++++++++
 .../selftests/bpf/progs/strncmp_bench.c       |  50 ++++++
 .../selftests/bpf/progs/strncmp_test.c        |  59 ++++++
 17 files changed, 604 insertions(+), 138 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/benchs/bench_strncmp.c
 create mode 100755 tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_strncmp.c
 create mode 100644 tools/testing/selftests/bpf/progs/strncmp_bench.c
 create mode 100644 tools/testing/selftests/bpf/progs/strncmp_test.c

-- 
2.29.2

[PATCH bpf-next 1/5] bpf: add bpf_strncmp helper

[Hou Tao]

The helper compares two strings: one string is a null-terminated
read-only string, and another string has const max storage size
but doesn't need to be null-terminated. It can be used to compare
file name in tracing or LSM program.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       | 11 +++++++++++
 kernel/bpf/helpers.c           | 16 ++++++++++++++++
 tools/include/uapi/linux/bpf.h | 11 +++++++++++
 4 files changed, 39 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a7cbc29b0994..685c8fe5c0be 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2165,6 +2165,7 @@ extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
 extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
 extern const struct bpf_func_proto bpf_kallsyms_lookup_name_proto;
 extern const struct bpf_func_proto bpf_find_vma_proto;
+extern const struct bpf_func_proto bpf_strncmp_proto;
 
 const struct bpf_func_proto *tracing_prog_func_proto(
   enum bpf_func_id func_id, const struct bpf_prog *prog);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a69e4b04ffeb..afdc52efa4a1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4957,6 +4957,16 @@ union bpf_attr {
  *		**-ENOENT** if *task->mm* is NULL, or no vma contains *addr*.
  *		**-EBUSY** if failed to try lock mmap_lock.
  *		**-EINVAL** for invalid **flags**.
+ *
+ * long bpf_strncmp(const char *s1, u32 s1_sz, const char *s2)
+ *	Description
+ *		Do strncmp() between **s1** and **s2**. **s1** doesn't need
+ *		to be null-terminated and **s1_sz** is the maximum storage
+ *		size of **s1**. **s2** must be a read-only string.
+ *	Return
+ *		An integer less than, equal to, or greater than zero
+ *		if the first **s1_sz** bytes of **s1** is found to be
+ *		less than, to match, or be greater than **s2**.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5140,6 +5150,7 @@ union bpf_attr {
 	FN(skc_to_unix_sock),		\
 	FN(kallsyms_lookup_name),	\
 	FN(find_vma),			\
+	FN(strncmp),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1ffd469c217f..a7d3a8d48e00 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -565,6 +565,20 @@ const struct bpf_func_proto bpf_strtoul_proto = {
 };
 #endif
 
+BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
+{
+	return strncmp(s1, s2, s1_sz);
+}
+
+const struct bpf_func_proto bpf_strncmp_proto = {
+	.func		= bpf_strncmp,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_MEM,
+	.arg2_type	= ARG_CONST_SIZE,
+	.arg3_type	= ARG_PTR_TO_CONST_STR,
+};
+
 BPF_CALL_4(bpf_get_ns_current_pid_tgid, u64, dev, u64, ino,
 	   struct bpf_pidns_info *, nsdata, u32, size)
 {
@@ -1378,6 +1392,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 		return &bpf_ringbuf_query_proto;
 	case BPF_FUNC_for_each_map_elem:
 		return &bpf_for_each_map_elem_proto;
+	case BPF_FUNC_strncmp:
+		return &bpf_strncmp_proto;
 	default:
 		break;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a69e4b04ffeb..afdc52efa4a1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4957,6 +4957,16 @@ union bpf_attr {
  *		**-ENOENT** if *task->mm* is NULL, or no vma contains *addr*.
  *		**-EBUSY** if failed to try lock mmap_lock.
  *		**-EINVAL** for invalid **flags**.
+ *
+ * long bpf_strncmp(const char *s1, u32 s1_sz, const char *s2)
+ *	Description
+ *		Do strncmp() between **s1** and **s2**. **s1** doesn't need
+ *		to be null-terminated and **s1_sz** is the maximum storage
+ *		size of **s1**. **s2** must be a read-only string.
+ *	Return
+ *		An integer less than, equal to, or greater than zero
+ *		if the first **s1_sz** bytes of **s1** is found to be
+ *		less than, to match, or be greater than **s2**.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5140,6 +5150,7 @@ union bpf_attr {
 	FN(skc_to_unix_sock),		\
 	FN(kallsyms_lookup_name),	\
 	FN(find_vma),			\
+	FN(strncmp),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.29.2

[PATCH bpf-next 2/5] selftests/bpf: fix checkpatch error on empty function parameter

[Hou Tao]

Fix checkpatch error: "ERROR: Bad function definition - void foo()
should probably be void foo(void)". Most replacements are done by
the following command:

  sed -i 's#\([a-z]\)()$#\1(void)#g' testing/selftests/bpf/benchs/*.c

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 tools/testing/selftests/bpf/bench.c           |  2 +-
 tools/testing/selftests/bpf/bench.h           |  9 +++----
 .../selftests/bpf/benchs/bench_count.c        |  2 +-
 .../selftests/bpf/benchs/bench_rename.c       | 16 ++++++-------
 .../selftests/bpf/benchs/bench_ringbufs.c     | 14 +++++------
 .../selftests/bpf/benchs/bench_trigger.c      | 24 +++++++++----------
 6 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index c75e7ee28746..ffb589b885c5 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -39,7 +39,7 @@ static int bump_memlock_rlimit(void)
 	return setrlimit(RLIMIT_MEMLOCK, &rlim_new);
 }
 
-void setup_libbpf()
+void setup_libbpf(void)
 {
 	int err;
 
diff --git a/tools/testing/selftests/bpf/bench.h b/tools/testing/selftests/bpf/bench.h
index 624c6b11501f..3fb8401b7314 100644
--- a/tools/testing/selftests/bpf/bench.h
+++ b/tools/testing/selftests/bpf/bench.h
@@ -38,8 +38,8 @@ struct bench_res {
 
 struct bench {
 	const char *name;
-	void (*validate)();
-	void (*setup)();
+	void (*validate)(void);
+	void (*setup)(void);
 	void *(*producer_thread)(void *ctx);
 	void *(*consumer_thread)(void *ctx);
 	void (*measure)(struct bench_res* res);
@@ -54,13 +54,14 @@ struct counter {
 extern struct env env;
 extern const struct bench *bench;
 
-void setup_libbpf();
+void setup_libbpf(void);
 void hits_drops_report_progress(int iter, struct bench_res *res, long delta_ns);
 void hits_drops_report_final(struct bench_res res[], int res_cnt);
 void false_hits_report_progress(int iter, struct bench_res *res, long delta_ns);
 void false_hits_report_final(struct bench_res res[], int res_cnt);
 
-static inline __u64 get_time_ns() {
+static inline __u64 get_time_ns(void)
+{
 	struct timespec t;
 
 	clock_gettime(CLOCK_MONOTONIC, &t);
diff --git a/tools/testing/selftests/bpf/benchs/bench_count.c b/tools/testing/selftests/bpf/benchs/bench_count.c
index befba7a82643..078972ce208e 100644
--- a/tools/testing/selftests/bpf/benchs/bench_count.c
+++ b/tools/testing/selftests/bpf/benchs/bench_count.c
@@ -36,7 +36,7 @@ static struct count_local_ctx {
 	struct counter *hits;
 } count_local_ctx;
 
-static void count_local_setup()
+static void count_local_setup(void)
 {
 	struct count_local_ctx *ctx = &count_local_ctx;
 
diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c
index c7ec114eca56..3c203b6d6a6e 100644
--- a/tools/testing/selftests/bpf/benchs/bench_rename.c
+++ b/tools/testing/selftests/bpf/benchs/bench_rename.c
@@ -11,7 +11,7 @@ static struct ctx {
 	int fd;
 } ctx;
 
-static void validate()
+static void validate(void)
 {
 	if (env.producer_cnt != 1) {
 		fprintf(stderr, "benchmark doesn't support multi-producer!\n");
@@ -43,7 +43,7 @@ static void measure(struct bench_res *res)
 	res->hits = atomic_swap(&ctx.hits.value, 0);
 }
 
-static void setup_ctx()
+static void setup_ctx(void)
 {
 	setup_libbpf();
 
@@ -71,36 +71,36 @@ static void attach_bpf(struct bpf_program *prog)
 	}
 }
 
-static void setup_base()
+static void setup_base(void)
 {
 	setup_ctx();
 }
 
-static void setup_kprobe()
+static void setup_kprobe(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.prog1);
 }
 
-static void setup_kretprobe()
+static void setup_kretprobe(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.prog2);
 }
 
-static void setup_rawtp()
+static void setup_rawtp(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.prog3);
 }
 
-static void setup_fentry()
+static void setup_fentry(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.prog4);
 }
 
-static void setup_fexit()
+static void setup_fexit(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.prog5);
diff --git a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
index 52d4a2f91dbd..da8593b3494a 100644
--- a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
+++ b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
@@ -88,12 +88,12 @@ const struct argp bench_ringbufs_argp = {
 
 static struct counter buf_hits;
 
-static inline void bufs_trigger_batch()
+static inline void bufs_trigger_batch(void)
 {
 	(void)syscall(__NR_getpgid);
 }
 
-static void bufs_validate()
+static void bufs_validate(void)
 {
 	if (env.consumer_cnt != 1) {
 		fprintf(stderr, "rb-libbpf benchmark doesn't support multi-consumer!\n");
@@ -132,7 +132,7 @@ static void ringbuf_libbpf_measure(struct bench_res *res)
 	res->drops = atomic_swap(&ctx->skel->bss->dropped, 0);
 }
 
-static struct ringbuf_bench *ringbuf_setup_skeleton()
+static struct ringbuf_bench *ringbuf_setup_skeleton(void)
 {
 	struct ringbuf_bench *skel;
 
@@ -167,7 +167,7 @@ static int buf_process_sample(void *ctx, void *data, size_t len)
 	return 0;
 }
 
-static void ringbuf_libbpf_setup()
+static void ringbuf_libbpf_setup(void)
 {
 	struct ringbuf_libbpf_ctx *ctx = &ringbuf_libbpf_ctx;
 	struct bpf_link *link;
@@ -223,7 +223,7 @@ static void ringbuf_custom_measure(struct bench_res *res)
 	res->drops = atomic_swap(&ctx->skel->bss->dropped, 0);
 }
 
-static void ringbuf_custom_setup()
+static void ringbuf_custom_setup(void)
 {
 	struct ringbuf_custom_ctx *ctx = &ringbuf_custom_ctx;
 	const size_t page_size = getpagesize();
@@ -352,7 +352,7 @@ static void perfbuf_measure(struct bench_res *res)
 	res->drops = atomic_swap(&ctx->skel->bss->dropped, 0);
 }
 
-static struct perfbuf_bench *perfbuf_setup_skeleton()
+static struct perfbuf_bench *perfbuf_setup_skeleton(void)
 {
 	struct perfbuf_bench *skel;
 
@@ -390,7 +390,7 @@ perfbuf_process_sample_raw(void *input_ctx, int cpu,
 	return LIBBPF_PERF_EVENT_CONT;
 }
 
-static void perfbuf_libbpf_setup()
+static void perfbuf_libbpf_setup(void)
 {
 	struct perfbuf_libbpf_ctx *ctx = &perfbuf_libbpf_ctx;
 	struct perf_event_attr attr;
diff --git a/tools/testing/selftests/bpf/benchs/bench_trigger.c b/tools/testing/selftests/bpf/benchs/bench_trigger.c
index 049a5ad56f65..7f957c55a3ca 100644
--- a/tools/testing/selftests/bpf/benchs/bench_trigger.c
+++ b/tools/testing/selftests/bpf/benchs/bench_trigger.c
@@ -11,7 +11,7 @@ static struct trigger_ctx {
 
 static struct counter base_hits;
 
-static void trigger_validate()
+static void trigger_validate(void)
 {
 	if (env.consumer_cnt != 1) {
 		fprintf(stderr, "benchmark doesn't support multi-consumer!\n");
@@ -45,7 +45,7 @@ static void trigger_measure(struct bench_res *res)
 	res->hits = atomic_swap(&ctx.skel->bss->hits, 0);
 }
 
-static void setup_ctx()
+static void setup_ctx(void)
 {
 	setup_libbpf();
 
@@ -67,37 +67,37 @@ static void attach_bpf(struct bpf_program *prog)
 	}
 }
 
-static void trigger_tp_setup()
+static void trigger_tp_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_tp);
 }
 
-static void trigger_rawtp_setup()
+static void trigger_rawtp_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_raw_tp);
 }
 
-static void trigger_kprobe_setup()
+static void trigger_kprobe_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_kprobe);
 }
 
-static void trigger_fentry_setup()
+static void trigger_fentry_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_fentry);
 }
 
-static void trigger_fentry_sleep_setup()
+static void trigger_fentry_sleep_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_fentry_sleep);
 }
 
-static void trigger_fmodret_setup()
+static void trigger_fmodret_setup(void)
 {
 	setup_ctx();
 	attach_bpf(ctx.skel->progs.bench_trigger_fmodret);
@@ -183,22 +183,22 @@ static void usetup(bool use_retprobe, bool use_nop)
 	ctx.skel->links.bench_trigger_uprobe = link;
 }
 
-static void uprobe_setup_with_nop()
+static void uprobe_setup_with_nop(void)
 {
 	usetup(false, true);
 }
 
-static void uretprobe_setup_with_nop()
+static void uretprobe_setup_with_nop(void)
 {
 	usetup(true, true);
 }
 
-static void uprobe_setup_without_nop()
+static void uprobe_setup_without_nop(void)
 {
 	usetup(false, false);
 }
 
-static void uretprobe_setup_without_nop()
+static void uretprobe_setup_without_nop(void)
 {
 	usetup(true, false);
 }
-- 
2.29.2

[PATCH bpf-next 3/5] selftests/bpf: factor out common helpers for benchmarks

[Hou Tao]

Five helpers are factored out to reduce boilerplate for
benchmark tests: do_getpgid(), getpgid_loop_producer(),
assert_single_consumer(), assert_single_producer() and
noop_consumer().

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 tools/testing/selftests/bpf/bench.c           | 13 +++++
 tools/testing/selftests/bpf/bench.h           | 25 +++++++++
 .../bpf/benchs/bench_bloom_filter_map.c       | 44 ++++-----------
 .../selftests/bpf/benchs/bench_count.c        | 14 +----
 .../selftests/bpf/benchs/bench_rename.c       | 27 +++------
 .../selftests/bpf/benchs/bench_ringbufs.c     |  7 +--
 .../selftests/bpf/benchs/bench_trigger.c      | 55 +++++++------------
 7 files changed, 81 insertions(+), 104 deletions(-)

diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index ffb589b885c5..681db8175fe1 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -134,6 +134,19 @@ void hits_drops_report_final(struct bench_res res[], int res_cnt)
 	       total_ops_mean, total_ops_stddev);
 }
 
+void *getpgid_loop_producer(void *ctx)
+{
+	while (true)
+		do_getpgid();
+
+	return NULL;
+}
+
+void *noop_consumer(void *ctx)
+{
+	return NULL;
+}
+
 const char *argp_program_version = "benchmark";
 const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
 const char argp_program_doc[] =
diff --git a/tools/testing/selftests/bpf/bench.h b/tools/testing/selftests/bpf/bench.h
index 3fb8401b7314..79913082b469 100644
--- a/tools/testing/selftests/bpf/bench.h
+++ b/tools/testing/selftests/bpf/bench.h
@@ -59,6 +59,8 @@ void hits_drops_report_progress(int iter, struct bench_res *res, long delta_ns);
 void hits_drops_report_final(struct bench_res res[], int res_cnt);
 void false_hits_report_progress(int iter, struct bench_res *res, long delta_ns);
 void false_hits_report_final(struct bench_res res[], int res_cnt);
+void *getpgid_loop_producer(void *ctx);
+void *noop_consumer(void *ctx);
 
 static inline __u64 get_time_ns(void)
 {
@@ -83,3 +85,26 @@ static inline long atomic_swap(long *value, long n)
 {
 	return __atomic_exchange_n(value, n, __ATOMIC_RELAXED);
 }
+
+static inline void assert_single_consumer(const char *name)
+{
+	if (env.consumer_cnt != 1) {
+		fprintf(stderr, "%s benchmark doesn't support multi-consumer!\n",
+			name);
+		exit(1);
+	}
+}
+
+static inline void assert_single_producer(const char *name)
+{
+	if (env.producer_cnt != 1) {
+		fprintf(stderr, "%s benchmark doesn't support multi-producer!\n",
+			name);
+		exit(1);
+	}
+}
+
+static inline void do_getpgid(void)
+{
+	(void)syscall(__NR_getpgid, 0);
+}
diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
index 5bcb8a8cdeb2..796553a7ab17 100644
--- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
+++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
@@ -107,24 +107,7 @@ const struct argp bench_bloom_map_argp = {
 
 static void validate(void)
 {
-	if (env.consumer_cnt != 1) {
-		fprintf(stderr,
-			"The bloom filter benchmarks do not support multi-consumer use\n");
-		exit(1);
-	}
-}
-
-static inline void trigger_bpf_program(void)
-{
-	syscall(__NR_getpgid);
-}
-
-static void *producer(void *input)
-{
-	while (true)
-		trigger_bpf_program();
-
-	return NULL;
+	assert_single_consumer("bloom filter");
 }
 
 static void *map_prepare_thread(void *arg)
@@ -421,17 +404,12 @@ static void measure(struct bench_res *res)
 	last_false_hits = total_false_hits;
 }
 
-static void *consumer(void *input)
-{
-	return NULL;
-}
-
 const struct bench bench_bloom_lookup = {
 	.name = "bloom-lookup",
 	.validate = validate,
 	.setup = bloom_lookup_setup,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -441,8 +419,8 @@ const struct bench bench_bloom_update = {
 	.name = "bloom-update",
 	.validate = validate,
 	.setup = bloom_update_setup,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -452,8 +430,8 @@ const struct bench bench_bloom_false_positive = {
 	.name = "bloom-false-positive",
 	.validate = validate,
 	.setup = false_positive_setup,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = false_hits_report_progress,
 	.report_final = false_hits_report_final,
@@ -463,8 +441,8 @@ const struct bench bench_hashmap_without_bloom = {
 	.name = "hashmap-without-bloom",
 	.validate = validate,
 	.setup = hashmap_no_bloom_setup,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -474,8 +452,8 @@ const struct bench bench_hashmap_with_bloom = {
 	.name = "hashmap-with-bloom",
 	.validate = validate,
 	.setup = hashmap_with_bloom_setup,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
diff --git a/tools/testing/selftests/bpf/benchs/bench_count.c b/tools/testing/selftests/bpf/benchs/bench_count.c
index 078972ce208e..6b4b1cdec8a7 100644
--- a/tools/testing/selftests/bpf/benchs/bench_count.c
+++ b/tools/testing/selftests/bpf/benchs/bench_count.c
@@ -18,11 +18,6 @@ static void *count_global_producer(void *input)
 	return NULL;
 }
 
-static void *count_global_consumer(void *input)
-{
-	return NULL;
-}
-
 static void count_global_measure(struct bench_res *res)
 {
 	struct count_global_ctx *ctx = &count_global_ctx;
@@ -56,11 +51,6 @@ static void *count_local_producer(void *input)
 	return NULL;
 }
 
-static void *count_local_consumer(void *input)
-{
-	return NULL;
-}
-
 static void count_local_measure(struct bench_res *res)
 {
 	struct count_local_ctx *ctx = &count_local_ctx;
@@ -74,7 +64,7 @@ static void count_local_measure(struct bench_res *res)
 const struct bench bench_count_global = {
 	.name = "count-global",
 	.producer_thread = count_global_producer,
-	.consumer_thread = count_global_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = count_global_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -84,7 +74,7 @@ const struct bench bench_count_local = {
 	.name = "count-local",
 	.setup = count_local_setup,
 	.producer_thread = count_local_producer,
-	.consumer_thread = count_local_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = count_local_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c
index 3c203b6d6a6e..89c2de8dbb4b 100644
--- a/tools/testing/selftests/bpf/benchs/bench_rename.c
+++ b/tools/testing/selftests/bpf/benchs/bench_rename.c
@@ -13,14 +13,8 @@ static struct ctx {
 
 static void validate(void)
 {
-	if (env.producer_cnt != 1) {
-		fprintf(stderr, "benchmark doesn't support multi-producer!\n");
-		exit(1);
-	}
-	if (env.consumer_cnt != 1) {
-		fprintf(stderr, "benchmark doesn't support multi-consumer!\n");
-		exit(1);
-	}
+	assert_single_producer("rename");
+	assert_single_consumer("rename");
 }
 
 static void *producer(void *input)
@@ -106,17 +100,12 @@ static void setup_fexit(void)
 	attach_bpf(ctx.skel->progs.prog5);
 }
 
-static void *consumer(void *input)
-{
-	return NULL;
-}
-
 const struct bench bench_rename_base = {
 	.name = "rename-base",
 	.validate = validate,
 	.setup = setup_base,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -127,7 +116,7 @@ const struct bench bench_rename_kprobe = {
 	.validate = validate,
 	.setup = setup_kprobe,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -138,7 +127,7 @@ const struct bench bench_rename_kretprobe = {
 	.validate = validate,
 	.setup = setup_kretprobe,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -149,7 +138,7 @@ const struct bench bench_rename_rawtp = {
 	.validate = validate,
 	.setup = setup_rawtp,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -160,7 +149,7 @@ const struct bench bench_rename_fentry = {
 	.validate = validate,
 	.setup = setup_fentry,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -171,7 +160,7 @@ const struct bench bench_rename_fexit = {
 	.validate = validate,
 	.setup = setup_fexit,
 	.producer_thread = producer,
-	.consumer_thread = consumer,
+	.consumer_thread = noop_consumer,
 	.measure = measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
diff --git a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
index da8593b3494a..f3cfb643140d 100644
--- a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
+++ b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
@@ -90,15 +90,12 @@ static struct counter buf_hits;
 
 static inline void bufs_trigger_batch(void)
 {
-	(void)syscall(__NR_getpgid);
+	do_getpgid();
 }
 
 static void bufs_validate(void)
 {
-	if (env.consumer_cnt != 1) {
-		fprintf(stderr, "rb-libbpf benchmark doesn't support multi-consumer!\n");
-		exit(1);
-	}
+	assert_single_consumer("rb-libbpf");
 
 	if (args.back2back && env.producer_cnt > 1) {
 		fprintf(stderr, "back-to-back mode makes sense only for single-producer case!\n");
diff --git a/tools/testing/selftests/bpf/benchs/bench_trigger.c b/tools/testing/selftests/bpf/benchs/bench_trigger.c
index 7f957c55a3ca..b3d9dae5f0a1 100644
--- a/tools/testing/selftests/bpf/benchs/bench_trigger.c
+++ b/tools/testing/selftests/bpf/benchs/bench_trigger.c
@@ -13,16 +13,13 @@ static struct counter base_hits;
 
 static void trigger_validate(void)
 {
-	if (env.consumer_cnt != 1) {
-		fprintf(stderr, "benchmark doesn't support multi-consumer!\n");
-		exit(1);
-	}
+	assert_single_consumer("trigger");
 }
 
 static void *trigger_base_producer(void *input)
 {
 	while (true) {
-		(void)syscall(__NR_getpgid);
+		do_getpgid();
 		atomic_inc(&base_hits.value);
 	}
 	return NULL;
@@ -33,13 +30,6 @@ static void trigger_base_measure(struct bench_res *res)
 	res->hits = atomic_swap(&base_hits.value, 0);
 }
 
-static void *trigger_producer(void *input)
-{
-	while (true)
-		(void)syscall(__NR_getpgid);
-	return NULL;
-}
-
 static void trigger_measure(struct bench_res *res)
 {
 	res->hits = atomic_swap(&ctx.skel->bss->hits, 0);
@@ -103,11 +93,6 @@ static void trigger_fmodret_setup(void)
 	attach_bpf(ctx.skel->progs.bench_trigger_fmodret);
 }
 
-static void *trigger_consumer(void *input)
-{
-	return NULL;
-}
-
 /* make sure call is not inlined and not avoided by compiler, so __weak and
  * inline asm volatile in the body of the function
  *
@@ -207,7 +192,7 @@ const struct bench bench_trig_base = {
 	.name = "trig-base",
 	.validate = trigger_validate,
 	.producer_thread = trigger_base_producer,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_base_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -217,8 +202,8 @@ const struct bench bench_trig_tp = {
 	.name = "trig-tp",
 	.validate = trigger_validate,
 	.setup = trigger_tp_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -228,8 +213,8 @@ const struct bench bench_trig_rawtp = {
 	.name = "trig-rawtp",
 	.validate = trigger_validate,
 	.setup = trigger_rawtp_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -239,8 +224,8 @@ const struct bench bench_trig_kprobe = {
 	.name = "trig-kprobe",
 	.validate = trigger_validate,
 	.setup = trigger_kprobe_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -250,8 +235,8 @@ const struct bench bench_trig_fentry = {
 	.name = "trig-fentry",
 	.validate = trigger_validate,
 	.setup = trigger_fentry_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -261,8 +246,8 @@ const struct bench bench_trig_fentry_sleep = {
 	.name = "trig-fentry-sleep",
 	.validate = trigger_validate,
 	.setup = trigger_fentry_sleep_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -272,8 +257,8 @@ const struct bench bench_trig_fmodret = {
 	.name = "trig-fmodret",
 	.validate = trigger_validate,
 	.setup = trigger_fmodret_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -283,7 +268,7 @@ const struct bench bench_trig_uprobe_base = {
 	.name = "trig-uprobe-base",
 	.setup = NULL, /* no uprobe/uretprobe is attached */
 	.producer_thread = uprobe_base_producer,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_base_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -293,7 +278,7 @@ const struct bench bench_trig_uprobe_with_nop = {
 	.name = "trig-uprobe-with-nop",
 	.setup = uprobe_setup_with_nop,
 	.producer_thread = uprobe_producer_with_nop,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -303,7 +288,7 @@ const struct bench bench_trig_uretprobe_with_nop = {
 	.name = "trig-uretprobe-with-nop",
 	.setup = uretprobe_setup_with_nop,
 	.producer_thread = uprobe_producer_with_nop,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -313,7 +298,7 @@ const struct bench bench_trig_uprobe_without_nop = {
 	.name = "trig-uprobe-without-nop",
 	.setup = uprobe_setup_without_nop,
 	.producer_thread = uprobe_producer_without_nop,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
@@ -323,7 +308,7 @@ const struct bench bench_trig_uretprobe_without_nop = {
 	.name = "trig-uretprobe-without-nop",
 	.setup = uretprobe_setup_without_nop,
 	.producer_thread = uprobe_producer_without_nop,
-	.consumer_thread = trigger_consumer,
+	.consumer_thread = noop_consumer,
 	.measure = trigger_measure,
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
-- 
2.29.2

Re: [PATCH bpf-next 3/5] selftests/bpf: factor out common helpers for benchmarks

[Andrii Nakryiko]

On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Five helpers are factored out to reduce boilerplate for
> benchmark tests: do_getpgid(), getpgid_loop_producer(),
> assert_single_consumer(), assert_single_producer() and
> noop_consumer().
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

Please drop this patch. All the stuff you are extracting into
"reusable" helpers is so trivial that it's not worth it. It just makes
it harder to follow each individual benchmark's setup.

>  tools/testing/selftests/bpf/bench.c           | 13 +++++
>  tools/testing/selftests/bpf/bench.h           | 25 +++++++++
>  .../bpf/benchs/bench_bloom_filter_map.c       | 44 ++++-----------
>  .../selftests/bpf/benchs/bench_count.c        | 14 +----
>  .../selftests/bpf/benchs/bench_rename.c       | 27 +++------
>  .../selftests/bpf/benchs/bench_ringbufs.c     |  7 +--
>  .../selftests/bpf/benchs/bench_trigger.c      | 55 +++++++------------
>  7 files changed, 81 insertions(+), 104 deletions(-)
>

[...]

Re: [PATCH bpf-next 3/5] selftests/bpf: factor out common helpers for benchmarks

[Hou Tao]

Hi,

On 12/7/2021 10:55 AM, Andrii Nakryiko wrote:
> On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>> Five helpers are factored out to reduce boilerplate for
>> benchmark tests: do_getpgid(), getpgid_loop_producer(),
>> assert_single_consumer(), assert_single_producer() and
>> noop_consumer().
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
> Please drop this patch. All the stuff you are extracting into
> "reusable" helpers is so trivial that it's not worth it. It just makes
> it harder to follow each individual benchmark's setup.
OK. Will do it v2.

[PATCH bpf-next 4/5] selftests/bpf: add benchmark for bpf_strncmp() helper

[Hou Tao]

Add benchmark to compare the performance between home-made strncmp()
in bpf program and bpf_strncmp() helper. In summary, the performance
win of bpf_strncmp() under x86-64 is greater than 18% when the compared
string length is greater than 64, and is 179% when the length is 4095.
Under arm64 the performance win is even bigger: 33% when the length
is greater than 64 and 600% when the length is 4095.

The following is the details:

no-helper-X: use home-made strncmp() to compare X-sized string
helper-Y: use bpf_strncmp() to compare Y-sized string

Under x86-64:

no-helper-1          3.504 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-1             3.347 ± 0.001M/s (drops 0.000 ± 0.000M/s)

no-helper-8          3.357 ± 0.001M/s (drops 0.000 ± 0.000M/s)
helper-8             3.307 ± 0.001M/s (drops 0.000 ± 0.000M/s)

no-helper-32         3.064 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-32            3.253 ± 0.001M/s (drops 0.000 ± 0.000M/s)

no-helper-64         2.563 ± 0.001M/s (drops 0.000 ± 0.000M/s)
helper-64            3.040 ± 0.001M/s (drops 0.000 ± 0.000M/s)

no-helper-128        1.975 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-128           2.641 ± 0.000M/s (drops 0.000 ± 0.000M/s)

no-helper-512        0.759 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-512           1.574 ± 0.000M/s (drops 0.000 ± 0.000M/s)

no-helper-2048       0.329 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-2048          0.602 ± 0.000M/s (drops 0.000 ± 0.000M/s)

no-helper-4095       0.117 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-4095          0.327 ± 0.000M/s (drops 0.000 ± 0.000M/s)

Under arm64:

no-helper-1          2.806 ± 0.004M/s (drops 0.000 ± 0.000M/s)
helper-1             2.819 ± 0.002M/s (drops 0.000 ± 0.000M/s)

no-helper-8          2.797 ± 0.109M/s (drops 0.000 ± 0.000M/s)
helper-8             2.786 ± 0.025M/s (drops 0.000 ± 0.000M/s)

no-helper-32         2.399 ± 0.011M/s (drops 0.000 ± 0.000M/s)
helper-32            2.703 ± 0.002M/s (drops 0.000 ± 0.000M/s)

no-helper-64         2.020 ± 0.015M/s (drops 0.000 ± 0.000M/s)
helper-64            2.702 ± 0.073M/s (drops 0.000 ± 0.000M/s)

no-helper-128        1.604 ± 0.001M/s (drops 0.000 ± 0.000M/s)
helper-128           2.516 ± 0.002M/s (drops 0.000 ± 0.000M/s)

no-helper-512        0.699 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-512           2.106 ± 0.003M/s (drops 0.000 ± 0.000M/s)

no-helper-2048       0.215 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-2048          1.223 ± 0.003M/s (drops 0.000 ± 0.000M/s)

no-helper-4095       0.112 ± 0.000M/s (drops 0.000 ± 0.000M/s)
helper-4095          0.796 ± 0.000M/s (drops 0.000 ± 0.000M/s)

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 tools/testing/selftests/bpf/Makefile          |   4 +-
 tools/testing/selftests/bpf/bench.c           |   6 +
 .../selftests/bpf/benchs/bench_strncmp.c      | 150 ++++++++++++++++++
 .../selftests/bpf/benchs/run_bench_strncmp.sh |  12 ++
 .../selftests/bpf/progs/strncmp_bench.c       |  50 ++++++
 5 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/benchs/bench_strncmp.c
 create mode 100755 tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh
 create mode 100644 tools/testing/selftests/bpf/progs/strncmp_bench.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 8ff7060fe754..7719924b01a3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -531,6 +531,7 @@ $(OUTPUT)/bench_trigger.o: $(OUTPUT)/trigger_bench.skel.h
 $(OUTPUT)/bench_ringbufs.o: $(OUTPUT)/ringbuf_bench.skel.h \
 			    $(OUTPUT)/perfbuf_bench.skel.h
 $(OUTPUT)/bench_bloom_filter_map.o: $(OUTPUT)/bloom_filter_bench.skel.h
+$(OUTPUT)/bench_strncmp.o: $(OUTPUT)/strncmp_bench.skel.h
 $(OUTPUT)/bench.o: bench.h testing_helpers.h $(BPFOBJ)
 $(OUTPUT)/bench: LDLIBS += -lm
 $(OUTPUT)/bench: $(OUTPUT)/bench.o \
@@ -540,7 +541,8 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o \
 		 $(OUTPUT)/bench_rename.o \
 		 $(OUTPUT)/bench_trigger.o \
 		 $(OUTPUT)/bench_ringbufs.o \
-		 $(OUTPUT)/bench_bloom_filter_map.o
+		 $(OUTPUT)/bench_bloom_filter_map.o \
+		 $(OUTPUT)/bench_strncmp.o
 	$(call msg,BINARY,,$@)
 	$(Q)$(CC) $(LDFLAGS) $(filter %.a %.o,$^) $(LDLIBS) -o $@
 
diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index 681db8175fe1..8f1a4351fb18 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -184,10 +184,12 @@ static const struct argp_option opts[] = {
 
 extern struct argp bench_ringbufs_argp;
 extern struct argp bench_bloom_map_argp;
+extern struct argp strncmp_argp;
 
 static const struct argp_child bench_parsers[] = {
 	{ &bench_ringbufs_argp, 0, "Ring buffers benchmark", 0 },
 	{ &bench_bloom_map_argp, 0, "Bloom filter map benchmark", 0 },
+	{ &strncmp_argp, 0, "Strncmp benchmark", 0 },
 	{},
 };
 
@@ -386,6 +388,8 @@ extern const struct bench bench_bloom_update;
 extern const struct bench bench_bloom_false_positive;
 extern const struct bench bench_hashmap_without_bloom;
 extern const struct bench bench_hashmap_with_bloom;
+extern const struct bench bench_strncmp_no_helper;
+extern const struct bench bench_strncmp_helper;
 
 static const struct bench *benchs[] = {
 	&bench_count_global,
@@ -417,6 +421,8 @@ static const struct bench *benchs[] = {
 	&bench_bloom_false_positive,
 	&bench_hashmap_without_bloom,
 	&bench_hashmap_with_bloom,
+	&bench_strncmp_no_helper,
+	&bench_strncmp_helper,
 };
 
 static void setup_benchmark()
diff --git a/tools/testing/selftests/bpf/benchs/bench_strncmp.c b/tools/testing/selftests/bpf/benchs/bench_strncmp.c
new file mode 100644
index 000000000000..57dea095e27a
--- /dev/null
+++ b/tools/testing/selftests/bpf/benchs/bench_strncmp.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <argp.h>
+#include "bench.h"
+#include "strncmp_bench.skel.h"
+
+struct strncmp_ctx {
+	struct strncmp_bench *skel;
+};
+
+struct strncmp_args {
+	u32 cmp_str_len;
+};
+
+static struct strncmp_args args = {
+	.cmp_str_len = 32,
+};
+
+static struct strncmp_ctx ctx;
+
+enum {
+	ARG_CMP_STR_LEN = 4000,
+};
+
+static const struct argp_option opts[] = {
+	{ "cmp-str-len", ARG_CMP_STR_LEN, "CMP_STR_LEN", 0,
+	  "Set the length of compared string" },
+	{},
+};
+
+static error_t strncmp_parse_arg(int key, char *arg, struct argp_state *state)
+{
+	switch (key) {
+	case ARG_CMP_STR_LEN:
+		args.cmp_str_len = strtoul(arg, NULL, 10);
+		if (!args.cmp_str_len ||
+		    args.cmp_str_len >= sizeof(ctx.skel->bss->str)) {
+			fprintf(stderr, "Invalid cmp str len (limit %zu)\n",
+				sizeof(ctx.skel->bss->str));
+			argp_usage(state);
+		}
+		break;
+	default:
+		return ARGP_ERR_UNKNOWN;
+	}
+
+	return 0;
+}
+
+const struct argp strncmp_argp = {
+	.options = opts,
+	.parser = strncmp_parse_arg,
+};
+
+static void strncmp_validate(void)
+{
+	assert_single_consumer("strncmp");
+}
+
+static void strncmp_setup(void)
+{
+	int err;
+	char *target;
+	size_t i, sz;
+
+	sz = sizeof(ctx.skel->rodata->target);
+	if (!sz || sz < sizeof(ctx.skel->bss->str)) {
+		fprintf(stderr, "invalid string size (target %zu, src %zu)\n",
+			sz, sizeof(ctx.skel->bss->str));
+		exit(1);
+	}
+
+	setup_libbpf();
+
+	ctx.skel = strncmp_bench__open();
+	if (!ctx.skel) {
+		fprintf(stderr, "failed to open skeleton\n");
+		exit(1);
+	}
+
+	srandom(time(NULL));
+	target = ctx.skel->rodata->target;
+	for (i = 0; i < sz - 1; i++)
+		target[i] = '1' + random() % 9;
+	target[sz - 1] = '\0';
+
+	ctx.skel->rodata->cmp_str_len = args.cmp_str_len;
+
+	memcpy(ctx.skel->bss->str, target, args.cmp_str_len);
+	ctx.skel->bss->str[args.cmp_str_len] = '\0';
+	/* Make bss->str < rodata->target */
+	ctx.skel->bss->str[args.cmp_str_len - 1] -= 1;
+
+	err = strncmp_bench__load(ctx.skel);
+	if (err) {
+		fprintf(stderr, "failed to load skeleton\n");
+		strncmp_bench__destroy(ctx.skel);
+		exit(1);
+	}
+}
+
+static void strncmp_attach_prog(struct bpf_program *prog)
+{
+	struct bpf_link *link;
+
+	link = bpf_program__attach(prog);
+	if (!link) {
+		fprintf(stderr, "failed to attach program!\n");
+		exit(1);
+	}
+}
+
+static void strncmp_no_helper_setup(void)
+{
+	strncmp_setup();
+	strncmp_attach_prog(ctx.skel->progs.strncmp_no_helper);
+}
+
+static void strncmp_helper_setup(void)
+{
+	strncmp_setup();
+	strncmp_attach_prog(ctx.skel->progs.strncmp_helper);
+}
+
+static void strncmp_measure(struct bench_res *res)
+{
+	res->hits = atomic_swap(&ctx.skel->bss->hits, 0);
+}
+
+const struct bench bench_strncmp_no_helper = {
+	.name = "strncmp-no-helper",
+	.validate = strncmp_validate,
+	.setup = strncmp_no_helper_setup,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
+	.measure = strncmp_measure,
+	.report_progress = hits_drops_report_progress,
+	.report_final = hits_drops_report_final,
+};
+
+const struct bench bench_strncmp_helper = {
+	.name = "strncmp-helper",
+	.validate = strncmp_validate,
+	.setup = strncmp_helper_setup,
+	.producer_thread = getpgid_loop_producer,
+	.consumer_thread = noop_consumer,
+	.measure = strncmp_measure,
+	.report_progress = hits_drops_report_progress,
+	.report_final = hits_drops_report_final,
+};
diff --git a/tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh b/tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh
new file mode 100755
index 000000000000..142697284b45
--- /dev/null
+++ b/tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh
@@ -0,0 +1,12 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source ./benchs/run_common.sh
+
+set -eufo pipefail
+
+for s in 1 8 64 512 2048 4095; do
+	for b in no-helper helper; do
+		summarize ${b}-${s} "$($RUN_BENCH --cmp-str-len=$s strncmp-${b})"
+	done
+done
diff --git a/tools/testing/selftests/bpf/progs/strncmp_bench.c b/tools/testing/selftests/bpf/progs/strncmp_bench.c
new file mode 100644
index 000000000000..18373a7df76e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/strncmp_bench.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <linux/types.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define STRNCMP_STR_SZ 4096
+
+/* Will be updated by benchmark before program loading */
+const volatile unsigned int cmp_str_len = 1;
+const char target[STRNCMP_STR_SZ];
+
+long hits = 0;
+char str[STRNCMP_STR_SZ];
+
+char _license[] SEC("license") = "GPL";
+
+static __always_inline int local_strncmp(const char *s1, unsigned int sz,
+					 const char *s2)
+{
+	int ret = 0;
+	unsigned int i;
+
+	for (i = 0; i < sz; i++) {
+		/* E.g. 0xff > 0x31 */
+		ret = (unsigned char)s1[i] - (unsigned char)s2[i];
+		if (ret || !s1[i])
+			break;
+	}
+
+	return ret;
+}
+
+SEC("tp/syscalls/sys_enter_getpgid")
+int strncmp_no_helper(void *ctx)
+{
+	if (local_strncmp(str, cmp_str_len + 1, target) < 0)
+		__sync_add_and_fetch(&hits, 1);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_getpgid")
+int strncmp_helper(void *ctx)
+{
+	if (bpf_strncmp(str, cmp_str_len + 1, target) < 0)
+		__sync_add_and_fetch(&hits, 1);
+	return 0;
+}
+
-- 
2.29.2

Re: [PATCH bpf-next 4/5] selftests/bpf: add benchmark for bpf_strncmp() helper

[Andrii Nakryiko]

On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Add benchmark to compare the performance between home-made strncmp()
> in bpf program and bpf_strncmp() helper. In summary, the performance
> win of bpf_strncmp() under x86-64 is greater than 18% when the compared
> string length is greater than 64, and is 179% when the length is 4095.
> Under arm64 the performance win is even bigger: 33% when the length
> is greater than 64 and 600% when the length is 4095.
>
> The following is the details:
>
> no-helper-X: use home-made strncmp() to compare X-sized string
> helper-Y: use bpf_strncmp() to compare Y-sized string
>
> Under x86-64:
>
> no-helper-1          3.504 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-1             3.347 ± 0.001M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-8          3.357 ± 0.001M/s (drops 0.000 ± 0.000M/s)
> helper-8             3.307 ± 0.001M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-32         3.064 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-32            3.253 ± 0.001M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-64         2.563 ± 0.001M/s (drops 0.000 ± 0.000M/s)
> helper-64            3.040 ± 0.001M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-128        1.975 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-128           2.641 ± 0.000M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-512        0.759 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-512           1.574 ± 0.000M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-2048       0.329 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-2048          0.602 ± 0.000M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-4095       0.117 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-4095          0.327 ± 0.000M/s (drops 0.000 ± 0.000M/s)
>
> Under arm64:
>
> no-helper-1          2.806 ± 0.004M/s (drops 0.000 ± 0.000M/s)
> helper-1             2.819 ± 0.002M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-8          2.797 ± 0.109M/s (drops 0.000 ± 0.000M/s)
> helper-8             2.786 ± 0.025M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-32         2.399 ± 0.011M/s (drops 0.000 ± 0.000M/s)
> helper-32            2.703 ± 0.002M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-64         2.020 ± 0.015M/s (drops 0.000 ± 0.000M/s)
> helper-64            2.702 ± 0.073M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-128        1.604 ± 0.001M/s (drops 0.000 ± 0.000M/s)
> helper-128           2.516 ± 0.002M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-512        0.699 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-512           2.106 ± 0.003M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-2048       0.215 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-2048          1.223 ± 0.003M/s (drops 0.000 ± 0.000M/s)
>
> no-helper-4095       0.112 ± 0.000M/s (drops 0.000 ± 0.000M/s)
> helper-4095          0.796 ± 0.000M/s (drops 0.000 ± 0.000M/s)
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  tools/testing/selftests/bpf/Makefile          |   4 +-
>  tools/testing/selftests/bpf/bench.c           |   6 +
>  .../selftests/bpf/benchs/bench_strncmp.c      | 150 ++++++++++++++++++
>  .../selftests/bpf/benchs/run_bench_strncmp.sh |  12 ++
>  .../selftests/bpf/progs/strncmp_bench.c       |  50 ++++++
>  5 files changed, 221 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/benchs/bench_strncmp.c
>  create mode 100755 tools/testing/selftests/bpf/benchs/run_bench_strncmp.sh
>  create mode 100644 tools/testing/selftests/bpf/progs/strncmp_bench.c
>

[...]

> diff --git a/tools/testing/selftests/bpf/progs/strncmp_bench.c b/tools/testing/selftests/bpf/progs/strncmp_bench.c
> new file mode 100644
> index 000000000000..18373a7df76e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/strncmp_bench.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
> +#include <linux/types.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#define STRNCMP_STR_SZ 4096
> +
> +/* Will be updated by benchmark before program loading */
> +const volatile unsigned int cmp_str_len = 1;
> +const char target[STRNCMP_STR_SZ];
> +
> +long hits = 0;
> +char str[STRNCMP_STR_SZ];
> +
> +char _license[] SEC("license") = "GPL";
> +
> +static __always_inline int local_strncmp(const char *s1, unsigned int sz,
> +                                        const char *s2)
> +{
> +       int ret = 0;
> +       unsigned int i;
> +
> +       for (i = 0; i < sz; i++) {
> +               /* E.g. 0xff > 0x31 */
> +               ret = (unsigned char)s1[i] - (unsigned char)s2[i];

I'm actually not sure if it will perform subtraction in unsigned form
(and thus you'll never have a negative result) and then cast to int,
or not. Why not cast to int instead of unsigned char to be sure?

> +               if (ret || !s1[i])
> +                       break;
> +       }
> +
> +       return ret;
> +}
> +
> +SEC("tp/syscalls/sys_enter_getpgid")
> +int strncmp_no_helper(void *ctx)
> +{
> +       if (local_strncmp(str, cmp_str_len + 1, target) < 0)
> +               __sync_add_and_fetch(&hits, 1);
> +       return 0;
> +}
> +
> +SEC("tp/syscalls/sys_enter_getpgid")
> +int strncmp_helper(void *ctx)
> +{
> +       if (bpf_strncmp(str, cmp_str_len + 1, target) < 0)
> +               __sync_add_and_fetch(&hits, 1);
> +       return 0;
> +}
> +
> --
> 2.29.2
>

Re: [PATCH bpf-next 4/5] selftests/bpf: add benchmark for bpf_strncmp() helper

[Hou Tao]

Hi,

On 12/7/2021 11:01 AM, Andrii Nakryiko wrote:
> On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>> Add benchmark to compare the performance between home-made strncmp()
>> in bpf program and bpf_strncmp() helper. In summary, the performance
>> win of bpf_strncmp() under x86-64 is greater than 18% when the compared
>> string length is greater than 64, and is 179% when the length is 4095.
>> Under arm64 the performance win is even bigger: 33% when the length
>> is greater than 64 and 600% when the length is 4095.
snip
>> +
>> +long hits = 0;
>> +char str[STRNCMP_STR_SZ];
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +static __always_inline int local_strncmp(const char *s1, unsigned int sz,
>> +                                        const char *s2)
>> +{
>> +       int ret = 0;
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < sz; i++) {
>> +               /* E.g. 0xff > 0x31 */
>> +               ret = (unsigned char)s1[i] - (unsigned char)s2[i];
> I'm actually not sure if it will perform subtraction in unsigned form
> (and thus you'll never have a negative result) and then cast to int,
> or not. Why not cast to int instead of unsigned char to be sure?
It is used to handle the character which is greater than or equal with 0x80.
When casting these character into int, the result will be a negative value,
the compare result will always be negative and it is wrong because
0xff should be greater than 0x31.

Re: [PATCH bpf-next 4/5] selftests/bpf: add benchmark for bpf_strncmp() helper

[Andrii Nakryiko]

On Wed, Dec 8, 2021 at 5:47 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Hi,
>
> On 12/7/2021 11:01 AM, Andrii Nakryiko wrote:
> > On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
> >> Add benchmark to compare the performance between home-made strncmp()
> >> in bpf program and bpf_strncmp() helper. In summary, the performance
> >> win of bpf_strncmp() under x86-64 is greater than 18% when the compared
> >> string length is greater than 64, and is 179% when the length is 4095.
> >> Under arm64 the performance win is even bigger: 33% when the length
> >> is greater than 64 and 600% when the length is 4095.
> snip
> >> +
> >> +long hits = 0;
> >> +char str[STRNCMP_STR_SZ];
> >> +
> >> +char _license[] SEC("license") = "GPL";
> >> +
> >> +static __always_inline int local_strncmp(const char *s1, unsigned int sz,
> >> +                                        const char *s2)
> >> +{
> >> +       int ret = 0;
> >> +       unsigned int i;
> >> +
> >> +       for (i = 0; i < sz; i++) {
> >> +               /* E.g. 0xff > 0x31 */
> >> +               ret = (unsigned char)s1[i] - (unsigned char)s2[i];
> > I'm actually not sure if it will perform subtraction in unsigned form
> > (and thus you'll never have a negative result) and then cast to int,
> > or not. Why not cast to int instead of unsigned char to be sure?
> It is used to handle the character which is greater than or equal with 0x80.
> When casting these character into int, the result will be a negative value,
> the compare result will always be negative and it is wrong because
> 0xff should be greater than 0x31.

I see about (unsigned char) cast, but I was worried that subtraction
result won't be negative I've tested with

$ cat test.c
#include <stdio.h>

int main() {
        int x = (unsigned char)190 - (unsigned char)255;
        printf("%d\n", x);
}


Seems like it behaves sanely (at least on this particular compiler),
so I'm fine with it.

[PATCH bpf-next 5/5] selftests/bpf: add test cases for bpf_strncmp()

[Hou Tao]

Four test cases are added:
(1) ensure the return value is expected
(2) ensure no const size is rejected
(3) ensure writable str is rejected
(4) ensure no null-terminated str is rejected

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/test_strncmp.c   | 170 ++++++++++++++++++
 .../selftests/bpf/progs/strncmp_test.c        |  59 ++++++
 2 files changed, 229 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_strncmp.c
 create mode 100644 tools/testing/selftests/bpf/progs/strncmp_test.c

diff --git a/tools/testing/selftests/bpf/prog_tests/test_strncmp.c b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c
new file mode 100644
index 000000000000..3ed54b55f96a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <test_progs.h>
+#include "strncmp_test.skel.h"
+
+static struct strncmp_test *strncmp_test_open_and_disable_autoload(void)
+{
+	struct strncmp_test *skel;
+	struct bpf_program *prog;
+
+	skel = strncmp_test__open();
+	if (libbpf_get_error(skel))
+		return skel;
+
+	bpf_object__for_each_program(prog, skel->obj)
+		bpf_program__set_autoload(prog, false);
+
+	return skel;
+}
+
+static inline int to_tristate_ret(int ret)
+{
+	if (ret > 0)
+		return 1;
+	if (ret < 0)
+		return -1;
+	return 0;
+}
+
+static int trigger_strncmp(const struct strncmp_test *skel)
+{
+	struct timespec wait = {.tv_sec = 0, .tv_nsec = 1};
+
+	nanosleep(&wait, NULL);
+	return to_tristate_ret(skel->bss->cmp_ret);
+}
+
+/*
+ * Compare str and target after making str[i] != target[i].
+ * When exp is -1, make str[i] < target[i] and delta is -1.
+ */
+static void strncmp_full_str_cmp(struct strncmp_test *skel, const char *name,
+				 int exp)
+{
+	size_t nr = sizeof(skel->bss->str);
+	char *str = skel->bss->str;
+	int delta = exp;
+	int got;
+	size_t i;
+
+	memcpy(str, skel->rodata->target, nr);
+	for (i = 0; i < nr - 1; i++) {
+		str[i] += delta;
+
+		got = trigger_strncmp(skel);
+		ASSERT_EQ(got, exp, name);
+
+		str[i] -= delta;
+	}
+}
+
+static void test_strncmp_ret(void)
+{
+	struct strncmp_test *skel;
+	int err, got;
+
+	skel = strncmp_test_open_and_disable_autoload();
+	if (!ASSERT_OK_PTR(skel, "strncmp_test open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.do_strncmp, true);
+
+	err = strncmp_test__load(skel);
+	if (!ASSERT_EQ(err, 0, "strncmp_test load"))
+		goto out;
+
+	err = strncmp_test__attach(skel);
+	if (!ASSERT_EQ(err, 0, "strncmp_test attach"))
+		goto out;
+
+	skel->bss->target_pid = getpid();
+
+	/* Empty str */
+	skel->bss->str[0] = '\0';
+	got = trigger_strncmp(skel);
+	ASSERT_EQ(got, -1, "strncmp: empty str");
+
+	/* Same string */
+	memcpy(skel->bss->str, skel->rodata->target, sizeof(skel->bss->str));
+	got = trigger_strncmp(skel);
+	ASSERT_EQ(got, 0, "strncmp: same str");
+
+	/* Not-null-termainted string  */
+	memcpy(skel->bss->str, skel->rodata->target, sizeof(skel->bss->str));
+	skel->bss->str[sizeof(skel->bss->str) - 1] = 'A';
+	got = trigger_strncmp(skel);
+	ASSERT_EQ(got, 1, "strncmp: not-null-term str");
+
+	strncmp_full_str_cmp(skel, "strncmp: less than", -1);
+	strncmp_full_str_cmp(skel, "strncmp: greater than", 1);
+out:
+	strncmp_test__destroy(skel);
+}
+
+static void test_strncmp_bad_not_const_str_size(void)
+{
+	struct strncmp_test *skel;
+	int err;
+
+	skel = strncmp_test_open_and_disable_autoload();
+	if (!ASSERT_OK_PTR(skel, "strncmp_test open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.strncmp_bad_not_const_str_size,
+				  true);
+
+	err = strncmp_test__load(skel);
+	ASSERT_ERR(err, "strncmp_test load bad_not_const_str_size");
+
+	strncmp_test__destroy(skel);
+}
+
+static void test_strncmp_bad_writable_target(void)
+{
+	struct strncmp_test *skel;
+	int err;
+
+	skel = strncmp_test_open_and_disable_autoload();
+	if (!ASSERT_OK_PTR(skel, "strncmp_test open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.strncmp_bad_writable_target,
+				  true);
+
+	err = strncmp_test__load(skel);
+	ASSERT_ERR(err, "strncmp_test load bad_writable_target");
+
+	strncmp_test__destroy(skel);
+}
+
+static void test_strncmp_bad_not_null_term_target(void)
+{
+	struct strncmp_test *skel;
+	int err;
+
+	skel = strncmp_test_open_and_disable_autoload();
+	if (!ASSERT_OK_PTR(skel, "strncmp_test open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.strncmp_bad_not_null_term_target,
+				  true);
+	skel->rodata->target[sizeof(skel->rodata->target) - 1] = 'A';
+
+	err = strncmp_test__load(skel);
+	ASSERT_ERR(err, "strncmp_test load bad_not_null_term_target");
+
+	strncmp_test__destroy(skel);
+}
+
+void test_test_strncmp(void)
+{
+	if (test__start_subtest("strncmp_ret"))
+		test_strncmp_ret();
+	if (test__start_subtest("strncmp_bad_not_const_str_size"))
+		test_strncmp_bad_not_const_str_size();
+	if (test__start_subtest("strncmp_bad_writable_target"))
+		test_strncmp_bad_writable_target();
+	if (test__start_subtest("strncmp_bad_not_null_term_target"))
+		test_strncmp_bad_not_null_term_target();
+}
diff --git a/tools/testing/selftests/bpf/progs/strncmp_test.c b/tools/testing/selftests/bpf/progs/strncmp_test.c
new file mode 100644
index 000000000000..8cdf950a0ce1
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/strncmp_test.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
+#include <stdbool.h>
+#include <linux/types.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define STRNCMP_STR_SZ 8
+
+const char target[STRNCMP_STR_SZ] = "EEEEEEE";
+
+char str[STRNCMP_STR_SZ];
+int cmp_ret = 0;
+int target_pid = 0;
+
+char bad_target[STRNCMP_STR_SZ];
+unsigned int bad_cmp_str_size = STRNCMP_STR_SZ;
+
+char _license[] SEC("license") = "GPL";
+
+static __always_inline bool called_by_target_pid(void)
+{
+	__u32 pid = bpf_get_current_pid_tgid() >> 32;
+
+	return pid == target_pid;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int do_strncmp(void *ctx)
+{
+	if (!called_by_target_pid())
+		return 0;
+
+	cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
+
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int strncmp_bad_not_const_str_size(void *ctx)
+{
+	cmp_ret = bpf_strncmp(str, bad_cmp_str_size, target);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int strncmp_bad_writable_target(void *ctx)
+{
+	cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, bad_target);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int strncmp_bad_not_null_term_target(void *ctx)
+{
+	cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
+	return 0;
+}
-- 
2.29.2

Re: [PATCH bpf-next 5/5] selftests/bpf: add test cases for bpf_strncmp()

[Andrii Nakryiko]

On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Four test cases are added:
> (1) ensure the return value is expected
> (2) ensure no const size is rejected
> (3) ensure writable str is rejected
> (4) ensure no null-terminated str is rejected
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  .../selftests/bpf/prog_tests/test_strncmp.c   | 170 ++++++++++++++++++
>  .../selftests/bpf/progs/strncmp_test.c        |  59 ++++++
>  2 files changed, 229 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_strncmp.c
>  create mode 100644 tools/testing/selftests/bpf/progs/strncmp_test.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_strncmp.c b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c
> new file mode 100644
> index 000000000000..3ed54b55f96a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
> +#include <test_progs.h>
> +#include "strncmp_test.skel.h"
> +
> +static struct strncmp_test *strncmp_test_open_and_disable_autoload(void)
> +{
> +       struct strncmp_test *skel;
> +       struct bpf_program *prog;
> +
> +       skel = strncmp_test__open();
> +       if (libbpf_get_error(skel))
> +               return skel;
> +
> +       bpf_object__for_each_program(prog, skel->obj)
> +               bpf_program__set_autoload(prog, false);

I think this is a wrong "code economy". You save few lines of code,
but make tests harder to follow. Just do 4 lines of code for each
subtest:

skel = strncmp_test__open();
if (!ASSERT_OK_PTR(skel, "skel_open"))
    return;

bpf_object__for_each_program(prog, skel->obj)
    bpf_program__set_autoload(prog, false);


It makes tests more self-contained and easier to follow. Also if some
tests need to do something slightly different it's easier to modify
them, as they are not coupled to some common helper. DRY is good where
it makes sense, but it also increases code coupling and more "jumping
around" in code, so it shouldn't be applied blindly.

> +
> +       return skel;
> +}
> +
> +static inline int to_tristate_ret(int ret)
> +{
> +       if (ret > 0)
> +               return 1;
> +       if (ret < 0)
> +               return -1;
> +       return 0;
> +}
> +
> +static int trigger_strncmp(const struct strncmp_test *skel)
> +{
> +       struct timespec wait = {.tv_sec = 0, .tv_nsec = 1};
> +
> +       nanosleep(&wait, NULL);

all the other tests are just doing usleep(1), why using this more verbose way?

> +       return to_tristate_ret(skel->bss->cmp_ret);
> +}
> +
> +/*
> + * Compare str and target after making str[i] != target[i].
> + * When exp is -1, make str[i] < target[i] and delta is -1.
> + */
> +static void strncmp_full_str_cmp(struct strncmp_test *skel, const char *name,
> +                                int exp)
> +{
> +       size_t nr = sizeof(skel->bss->str);
> +       char *str = skel->bss->str;
> +       int delta = exp;
> +       int got;
> +       size_t i;
> +
> +       memcpy(str, skel->rodata->target, nr);
> +       for (i = 0; i < nr - 1; i++) {
> +               str[i] += delta;
> +
> +               got = trigger_strncmp(skel);
> +               ASSERT_EQ(got, exp, name);
> +
> +               str[i] -= delta;
> +       }
> +}
> +

[...]

> diff --git a/tools/testing/selftests/bpf/progs/strncmp_test.c b/tools/testing/selftests/bpf/progs/strncmp_test.c
> new file mode 100644
> index 000000000000..8cdf950a0ce1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/strncmp_test.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */
> +#include <stdbool.h>
> +#include <linux/types.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#define STRNCMP_STR_SZ 8
> +
> +const char target[STRNCMP_STR_SZ] = "EEEEEEE";
> +
> +char str[STRNCMP_STR_SZ];
> +int cmp_ret = 0;
> +int target_pid = 0;
> +
> +char bad_target[STRNCMP_STR_SZ];
> +unsigned int bad_cmp_str_size = STRNCMP_STR_SZ;
> +
> +char _license[] SEC("license") = "GPL";
> +
> +static __always_inline bool called_by_target_pid(void)
> +{
> +       __u32 pid = bpf_get_current_pid_tgid() >> 32;
> +
> +       return pid == target_pid;
> +}

again, what's the point of this helper? it's used once and you'd
actually save the code by doing the following inline:

if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
    return 0;

> +
> +SEC("tp/syscalls/sys_enter_nanosleep")
> +int do_strncmp(void *ctx)
> +{
> +       if (!called_by_target_pid())
> +               return 0;
> +
> +       cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
> +
> +       return 0;
> +}
> +
> +SEC("tp/syscalls/sys_enter_nanosleep")
> +int strncmp_bad_not_const_str_size(void *ctx)
> +{

probably worth leaving a short comment explaining that this program
should fail because ...

> +       cmp_ret = bpf_strncmp(str, bad_cmp_str_size, target);
> +       return 0;
> +}
> +
> +SEC("tp/syscalls/sys_enter_nanosleep")
> +int strncmp_bad_writable_target(void *ctx)
> +{
> +       cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, bad_target);
> +       return 0;
> +}
> +
> +SEC("tp/syscalls/sys_enter_nanosleep")
> +int strncmp_bad_not_null_term_target(void *ctx)
> +{
> +       cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
> +       return 0;
> +}
> --
> 2.29.2
>

Re: [PATCH bpf-next 5/5] selftests/bpf: add test cases for bpf_strncmp()

[Hou Tao]

Hi,
On 12/7/2021 11:09 AM, Andrii Nakryiko wrote:
>> +static struct strncmp_test *strncmp_test_open_and_disable_autoload(void)
>> +{
>> +       struct strncmp_test *skel;
>> +       struct bpf_program *prog;
>> +
>> +       skel = strncmp_test__open();
>> +       if (libbpf_get_error(skel))
>> +               return skel;
>> +
>> +       bpf_object__for_each_program(prog, skel->obj)
>> +               bpf_program__set_autoload(prog, false);
> I think this is a wrong "code economy". You save few lines of code,
> but make tests harder to follow. Just do 4 lines of code for each
> subtest:
>
> skel = strncmp_test__open();
> if (!ASSERT_OK_PTR(skel, "skel_open"))
>     return;
>
> bpf_object__for_each_program(prog, skel->obj)
>     bpf_program__set_autoload(prog, false);
>
>
> It makes tests more self-contained and easier to follow. Also if some
> tests need to do something slightly different it's easier to modify
> them, as they are not coupled to some common helper. DRY is good where
> it makes sense, but it also increases code coupling and more "jumping
> around" in code, so it shouldn't be applied blindly.
Thanks for your suggestion on DRY topic. Will do in v2.
> +
> +static int trigger_strncmp(const struct strncmp_test *skel)
> +{
> +       struct timespec wait = {.tv_sec = 0, .tv_nsec = 1};
> +
> +       nanosleep(&wait, NULL);
> all the other tests are just doing usleep(1), why using this more verbose way?
Will do in v2.
>> +
>> +static __always_inline bool called_by_target_pid(void)
>> +{
>> +       __u32 pid = bpf_get_current_pid_tgid() >> 32;
>> +
>> +       return pid == target_pid;
>> +}
> again, what's the point of this helper? it's used once and you'd
> actually save the code by doing the following inline:
>
> if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
>     return 0;
Will do in v2.
>> +
>> +SEC("tp/syscalls/sys_enter_nanosleep")
>> +int do_strncmp(void *ctx)
>> +{
>> +       if (!called_by_target_pid())
>> +               return 0;
>> +
>> +       cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
>> +
>> +       return 0;
>> +}
>> +
>> +SEC("tp/syscalls/sys_enter_nanosleep")
>> +int strncmp_bad_not_const_str_size(void *ctx)
>> +{
> probably worth leaving a short comment explaining that this program
> should fail because ...
OK. Will do in v2.

Regards,
Tao

Re: [PATCH bpf-next 0/5] introduce bpf_strncmp() helper

[Alexei Starovoitov]

On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Hi,
>
> The motivation for introducing bpf_strncmp() helper comes from
> two aspects:
>
> (1) clang doesn't always replace strncmp() automatically
> In tracing program, sometimes we need to using a home-made
> strncmp() to check whether or not the file name is expected.
>
> (2) the performance of home-made strncmp is not so good
> As shown in the benchmark in patch #4, the performance of
> bpf_strncmp() helper is 18% or 33% better than home-made strncmp()
> under x86-64 or arm64 when the compared string length is 64. When
> the string length grows to 4095, the performance win will be
> 179% or 600% under x86-64 or arm64.

I think 'home made' strncmp could have been written
differently. I bet in bpf assembly it would be much closer
in performance if not the same,
but the helper is useful.
The patch set doesn't apply cleanly.
Pls respin.