2021-10-21 03:48:46

by Yafang Shao

Subject: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16

The hard-coded 16 is used in various bpf progs. These progs get task
comm either via bpf_get_current_comm() or prctl() or
bpf_core_read_str(), all of which can work well even if the task comm size
is changed.
Below is the detailed information,

bpf_get_current_comm:
progs/test_ringbuf.c
progs/test_ringbuf_multi.c

prctl:
prog_tests/test_overhead.c
prog_tests/trampoline_count.c

bpf_core_read_str:
progs/test_core_reloc_kernel.c
progs/test_sk_storage_tracing.c

We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it
more grepable.

Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
tools/testing/selftests/bpf/Makefile | 2 +-
tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++-
tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++-
.../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++-
tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++-
tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++-
tools/testing/selftests/bpf/progs/profiler.h | 7 ++++---
tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++----
tools/testing/selftests/bpf/progs/pyperf.h | 4 ++--
tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++---
.../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++-
tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++-
tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++-
.../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++--
tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++---
tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++--
tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++--
17 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 799b88152e9e..5e72d783d3fe 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)

CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
- -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
+ -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \
-I$(abspath $(OUTPUT)/../usr/include)

CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index 4706cee84360..ac82d57c09dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -12,6 +12,7 @@
#include <sys/sysinfo.h>
#include <linux/perf_event.h>
#include <linux/ring_buffer.h>
+#include <linux/sched/task.h>
#include "test_ringbuf.lskel.h"

#define EDONE 7777
@@ -22,7 +23,7 @@ struct sample {
int pid;
int seq;
long value;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

static int sample_cnt;
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
index 167cd8a2edfd..f0748305ffd6 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
@@ -2,6 +2,7 @@
#define _GNU_SOURCE
#include <test_progs.h>
#include <sys/epoll.h>
+#include <linux/sched/task.h>
#include "test_ringbuf_multi.skel.h"

static int duration = 0;
@@ -10,7 +11,7 @@ struct sample {
int pid;
int seq;
long value;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

static int process_sample(void *ctx, void *data, size_t len)
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_storage_tracing.c b/tools/testing/selftests/bpf/prog_tests/sk_storage_tracing.c
index 2b392590e8ca..f77d3b44ed35 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_storage_tracing.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_storage_tracing.c
@@ -4,6 +4,7 @@
#include <sys/types.h>
#include <bpf/bpf.h>
#include <bpf/libbpf.h>
+#include <linux/sched/task.h>
#include "test_progs.h"
#include "network_helpers.h"
#include "test_sk_storage_trace_itself.skel.h"
@@ -15,7 +16,7 @@
struct sk_stg {
__u32 pid;
__u32 last_notclose_state;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

static struct test_sk_storage_tracing *skel;
diff --git a/tools/testing/selftests/bpf/prog_tests/test_overhead.c b/tools/testing/selftests/bpf/prog_tests/test_overhead.c
index 123c68c1917d..133987217f57 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_overhead.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_overhead.c
@@ -4,6 +4,7 @@
#include <sched.h>
#include <sys/prctl.h>
#include <test_progs.h>
+#include <linux/sched/task.h>

#define MAX_CNT 100000

@@ -67,7 +68,7 @@ void test_test_overhead(void)
struct bpf_object *obj;
struct bpf_link *link;
int err, duration = 0;
- char comm[16] = {};
+ char comm[TASK_COMM_LEN_16] = {};

if (CHECK_FAIL(prctl(PR_GET_NAME, comm, 0L, 0L, 0L)))
return;
diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
index d7f5a931d7f3..4765b2ebd219 100644
--- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
+++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
@@ -3,6 +3,7 @@
#include <sched.h>
#include <sys/prctl.h>
#include <test_progs.h>
+#include <linux/sched/task.h>

#define MAX_TRAMP_PROGS 38

@@ -50,7 +51,7 @@ void test_trampoline_count(void)
int err, i = 0, duration = 0;
struct bpf_object *obj;
struct bpf_link *link;
- char comm[16] = {};
+ char comm[TASK_COMM_LEN_16] = {};

/* attach 'allowed' trampoline programs */
for (i = 0; i < MAX_TRAMP_PROGS; i++) {
diff --git a/tools/testing/selftests/bpf/progs/profiler.h b/tools/testing/selftests/bpf/progs/profiler.h
index 3bac4fdd4bdf..7ffc801d790b 100644
--- a/tools/testing/selftests/bpf/progs/profiler.h
+++ b/tools/testing/selftests/bpf/progs/profiler.h
@@ -2,7 +2,8 @@
/* Copyright (c) 2020 Facebook */
#pragma once

-#define TASK_COMM_LEN 16
+#include <linux/sched/task.h>
+
#define MAX_ANCESTORS 4
#define MAX_PATH 256
#define KILL_TARGET_LEN 64
@@ -14,7 +15,7 @@
#define MAX_FILEPATH_LENGTH (MAX_PATH_DEPTH * MAX_PATH)
#define MAX_CGROUPS_PATH_DEPTH 8

-#define MAX_METADATA_PAYLOAD_LEN TASK_COMM_LEN
+#define MAX_METADATA_PAYLOAD_LEN TASK_COMM_LEN_16

#define MAX_CGROUP_PAYLOAD_LEN \
(MAX_PATH * 2 + (MAX_PATH * MAX_CGROUPS_PATH_DEPTH))
@@ -25,7 +26,7 @@
(MAX_METADATA_PAYLOAD_LEN + MAX_CGROUP_PAYLOAD_LEN + CTL_MAXNAME + MAX_PATH)

#define MAX_KILL_PAYLOAD_LEN \
- (MAX_METADATA_PAYLOAD_LEN + MAX_CGROUP_PAYLOAD_LEN + TASK_COMM_LEN + \
+ (MAX_METADATA_PAYLOAD_LEN + MAX_CGROUP_PAYLOAD_LEN + TASK_COMM_LEN_16 + \
KILL_TARGET_LEN)

#define MAX_EXEC_PAYLOAD_LEN \
diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
index 4896fdf816f7..fad39075e5ce 100644
--- a/tools/testing/selftests/bpf/progs/profiler.inc.h
+++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
@@ -344,9 +344,9 @@ static INLINE void* populate_var_metadata(struct var_metadata_t* metadata,
metadata->start_time = BPF_CORE_READ(task, start_time);
metadata->comm_length = 0;

- size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN, &task->comm);
+ size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN_16, &task->comm);
barrier_var(comm_length);
- if (comm_length <= TASK_COMM_LEN) {
+ if (comm_length <= TASK_COMM_LEN_16) {
barrier_var(comm_length);
metadata->comm_length = comm_length;
payload += comm_length;
@@ -648,9 +648,9 @@ int raw_tracepoint__sched_process_exit(void* ctx)
kill_data->kill_target_name_length = 0;
kill_data->kill_target_cgroup_proc_length = 0;

- size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN, &task->comm);
+ size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN_16, &task->comm);
barrier_var(comm_length);
- if (comm_length <= TASK_COMM_LEN) {
+ if (comm_length <= TASK_COMM_LEN_16) {
barrier_var(comm_length);
kill_data->kill_target_name_length = comm_length;
payload += comm_length;
diff --git a/tools/testing/selftests/bpf/progs/pyperf.h b/tools/testing/selftests/bpf/progs/pyperf.h
index 2fb7adafb6b6..05cb40268887 100644
--- a/tools/testing/selftests/bpf/progs/pyperf.h
+++ b/tools/testing/selftests/bpf/progs/pyperf.h
@@ -6,11 +6,11 @@
#include <stddef.h>
#include <stdbool.h>
#include <linux/bpf.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

#define FUNCTION_NAME_LEN 64
#define FILE_NAME_LEN 128
-#define TASK_COMM_LEN 16

typedef struct {
int PyThreadState_frame;
@@ -43,7 +43,7 @@ typedef struct {
typedef struct {
uint32_t pid;
uint32_t tid;
- char comm[TASK_COMM_LEN];
+ char comm[TASK_COMM_LEN_16];
int32_t kernel_stack_id;
int32_t user_stack_id;
bool thread_current;
diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
index 7de534f38c3f..acfe929fd32d 100644
--- a/tools/testing/selftests/bpf/progs/strobemeta.h
+++ b/tools/testing/selftests/bpf/progs/strobemeta.h
@@ -8,12 +8,12 @@
#include <linux/ptrace.h>
#include <linux/sched.h>
#include <linux/types.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

typedef uint32_t pid_t;
struct task_struct {};

-#define TASK_COMM_LEN 16
#define PERF_MAX_STACK_DEPTH 127

#define STROBE_TYPE_INVALID 0
@@ -189,7 +189,7 @@ struct strobemeta_payload {

struct strobelight_bpf_sample {
uint64_t ktime;
- char comm[TASK_COMM_LEN];
+ char comm[TASK_COMM_LEN_16];
pid_t pid;
int user_stack_id;
int kernel_stack_id;
@@ -520,7 +520,7 @@ int on_event(struct pt_regs *ctx) {
return 0; /* this will never happen */

sample->pid = pid;
- bpf_get_current_comm(&sample->comm, TASK_COMM_LEN);
+ bpf_get_current_comm(&sample->comm, TASK_COMM_LEN_16);
ktime_ns = bpf_ktime_get_ns();
sample->ktime = ktime_ns;

diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
index 145028b52ad8..33bf5b1058e5 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
@@ -4,6 +4,7 @@
#include <linux/bpf.h>
#include <stdint.h>
#include <stdbool.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>

@@ -26,7 +27,7 @@ struct core_reloc_kernel_output {
struct task_struct {
int pid;
int tgid;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
struct task_struct *group_leader;
};

diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c
index eaa7d9dba0be..15bb2087371e 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c
@@ -2,6 +2,7 @@
// Copyright (c) 2020 Facebook

#include <linux/bpf.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

char _license[] SEC("license") = "GPL";
@@ -10,7 +11,7 @@ struct sample {
int pid;
int seq;
long value;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

struct {
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
index 197b86546dca..88c7f65a8f3f 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
@@ -2,6 +2,7 @@
// Copyright (c) 2020 Facebook

#include <linux/bpf.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

char _license[] SEC("license") = "GPL";
@@ -10,7 +11,7 @@ struct sample {
int pid;
int seq;
long value;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

struct ringbuf_map {
diff --git a/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c b/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
index 8e94e5c080aa..cd965e9f22f0 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
@@ -2,6 +2,7 @@
/* Copyright (c) 2020 Facebook */

#include <vmlinux.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_core_read.h>
#include <bpf/bpf_helpers.h>
@@ -9,7 +10,7 @@
struct sk_stg {
__u32 pid;
__u32 last_notclose_state;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};

struct {
@@ -27,7 +28,7 @@ struct {
__type(value, int);
} del_sk_stg_map SEC(".maps");

-char task_comm[16] = "";
+char task_comm[TASK_COMM_LEN_16] = "";

SEC("tp_btf/inet_sock_set_state")
int BPF_PROG(trace_inet_sock_set_state, struct sock *sk, int oldstate,
diff --git a/tools/testing/selftests/bpf/progs/test_skb_helpers.c b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
index bb3fbf1a29e3..cbe134eab8fe 100644
--- a/tools/testing/selftests/bpf/progs/test_skb_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
@@ -1,10 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
#include "vmlinux.h"
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>

-#define TEST_COMM_LEN 16
-
struct {
__uint(type, BPF_MAP_TYPE_CGROUP_ARRAY);
__uint(max_entries, 1);
@@ -18,7 +17,7 @@ SEC("classifier/test_skb_helpers")
int test_skb_helpers(struct __sk_buff *skb)
{
struct task_struct *task;
- char comm[TEST_COMM_LEN];
+ char comm[TASK_COMM_LEN_16];
__u32 tpid;

task = (struct task_struct *)bpf_get_current_task();
diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
index 00ed48672620..e4938b3cdf7a 100644
--- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
+++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
@@ -2,6 +2,7 @@
// Copyright (c) 2018 Facebook

#include <linux/bpf.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

#ifndef PERF_MAX_STACK_DEPTH
@@ -41,11 +42,11 @@ struct {
/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
struct sched_switch_args {
unsigned long long pad;
- char prev_comm[16];
+ char prev_comm[TASK_COMM_LEN_16];
int prev_pid;
int prev_prio;
long long prev_state;
- char next_comm[16];
+ char next_comm[TASK_COMM_LEN_16];
int next_pid;
int next_prio;
};
diff --git a/tools/testing/selftests/bpf/progs/test_tracepoint.c b/tools/testing/selftests/bpf/progs/test_tracepoint.c
index 4b825ee122cf..eb6e84ebf704 100644
--- a/tools/testing/selftests/bpf/progs/test_tracepoint.c
+++ b/tools/testing/selftests/bpf/progs/test_tracepoint.c
@@ -2,16 +2,17 @@
// Copyright (c) 2017 Facebook

#include <linux/bpf.h>
+#include <linux/sched/task.h>
#include <bpf/bpf_helpers.h>

/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
struct sched_switch_args {
unsigned long long pad;
- char prev_comm[16];
+ char prev_comm[TASK_COMM_LEN_16];
int prev_pid;
int prev_prio;
long long prev_state;
- char next_comm[16];
+ char next_comm[TASK_COMM_LEN_16];
int next_pid;
int next_prio;
};
--
2.17.1

2021-10-21 22:45:43

by Andrii Nakryiko

Subject: Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16

On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <[email protected]> wrote:
>
> The hard-coded 16 is used in various bpf progs. These progs get task
> comm either via bpf_get_current_comm() or prctl() or
> bpf_core_read_str(), all of which can work well even if the task comm size
> is changed.
> Below is the detailed information,
>
> bpf_get_current_comm:
> progs/test_ringbuf.c
> progs/test_ringbuf_multi.c
>
> prctl:
> prog_tests/test_overhead.c
> prog_tests/trampoline_count.c
>
> bpf_core_read_str:
> progs/test_core_reloc_kernel.c
> progs/test_sk_storage_tracing.c
>
> We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it
> more grepable.
>
> Signed-off-by: Yafang Shao <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++-
> tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++-
> .../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++-
> tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++-
> tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++-
> tools/testing/selftests/bpf/progs/profiler.h | 7 ++++---
> tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++----
> tools/testing/selftests/bpf/progs/pyperf.h | 4 ++--
> tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++---
> .../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++-
> tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++-
> tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++-
> .../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++--
> tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++---
> tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++--
> tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++--
> 17 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 799b88152e9e..5e72d783d3fe 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
>
> CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
> BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
> - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
> + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \

please don't add new include paths unnecessarily. See my comment on
another patch, if you add those new constants as enums, they will be
automatically available in vmlinux BTF and thus in auto-generated
vmlinux.h header (for those programs using it). For others, I'd just
leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate.

> -I$(abspath $(OUTPUT)/../usr/include)
>
> CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> index 4706cee84360..ac82d57c09dc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> @@ -12,6 +12,7 @@
> #include <sys/sysinfo.h>
> #include <linux/perf_event.h>
> #include <linux/ring_buffer.h>
> +#include <linux/sched/task.h>
> #include "test_ringbuf.lskel.h"
>
> #define EDONE 7777
> @@ -22,7 +23,7 @@ struct sample {
> int pid;
> int seq;
> long value;
> - char comm[16];
> + char comm[TASK_COMM_LEN_16];

how much value is in this "grep-ability", really? I'm not convinced
all this code churn is justified.

> };
>
> static int sample_cnt;
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> index 167cd8a2edfd..f0748305ffd6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> @@ -2,6 +2,7 @@
> #define _GNU_SOURCE
> #include <test_progs.h>
> #include <sys/epoll.h>
> +#include <linux/sched/task.h>
> #include "test_ringbuf_multi.skel.h"
>
> static int duration = 0;

[...]

2021-10-22 06:40:48

by Yafang Shao

Subject: Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16

On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <[email protected]> wrote:
> >
> > The hard-coded 16 is used in various bpf progs. These progs get task
> > comm either via bpf_get_current_comm() or prctl() or
> > bpf_core_read_str(), all of which can work well even if the task comm size
> > is changed.
> > Below is the detailed information,
> >
> > bpf_get_current_comm:
> > progs/test_ringbuf.c
> > progs/test_ringbuf_multi.c
> >
> > prctl:
> > prog_tests/test_overhead.c
> > prog_tests/trampoline_count.c
> >
> > bpf_core_read_str:
> > progs/test_core_reloc_kernel.c
> > progs/test_sk_storage_tracing.c
> >
> > We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it
> > more grepable.
> >
> > Signed-off-by: Yafang Shao <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > tools/testing/selftests/bpf/Makefile | 2 +-
> > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++-
> > .../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++-
> > tools/testing/selftests/bpf/progs/profiler.h | 7 ++++---
> > tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++----
> > tools/testing/selftests/bpf/progs/pyperf.h | 4 ++--
> > tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++---
> > .../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++-
> > tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++-
> > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++-
> > .../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++--
> > tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++---
> > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++--
> > tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++--
> > 17 files changed, 41 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 799b88152e9e..5e72d783d3fe 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
> >
> > CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
> > BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
> > - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
> > + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \
>
> please don't add new include paths unnecessarily. See my comment on
> another patch, if you add those new constants as enums, they will be
> automatically available in vmlinux BTF and thus in auto-generated
> vmlinux.h header (for those programs using it).

Yes, after converting it to enum, the BPF programs can get it from the
generated vmlinux.h.

> For others, I'd just
> leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate.
>

It seems not all the BPF programs can include the vmlinux.h.
What we really care about here is the copy of task comm should be with
a nul terminator, if we can assure it, then the size used by the BPF
is not important.
I have checked the copy of task comm in all these BPF programs one by
one, and replaced the unsafe bpf_probe_read_kernel() with
bpf_probe_read_kernel_str(), after that change, I think we can leave
hard-coded 16 for the progs which can't include vmlinux.h.

> > -I$(abspath $(OUTPUT)/../usr/include)
> >
> > CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > index 4706cee84360..ac82d57c09dc 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > @@ -12,6 +12,7 @@
> > #include <sys/sysinfo.h>
> > #include <linux/perf_event.h>
> > #include <linux/ring_buffer.h>
> > +#include <linux/sched/task.h>
> > #include "test_ringbuf.lskel.h"
> >
> > #define EDONE 7777
> > @@ -22,7 +23,7 @@ struct sample {
> > int pid;
> > int seq;
> > long value;
> > - char comm[16];
> > + char comm[TASK_COMM_LEN_16];
>
> how much value is in this "grep-ability", really? I'm not convinced
> all this code churn is justified.
>
> > };
> >
> > static int sample_cnt;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > index 167cd8a2edfd..f0748305ffd6 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > @@ -2,6 +2,7 @@
> > #define _GNU_SOURCE
> > #include <test_progs.h>
> > #include <sys/epoll.h>
> > +#include <linux/sched/task.h>
> > #include "test_ringbuf_multi.skel.h"
> >
> > static int duration = 0;
>
> [...]


--
Thanks
Yafang

2021-10-22 23:43:46

by Andrii Nakryiko

Subject: Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16

On Thu, Oct 21, 2021 at 11:36 PM Yafang Shao <[email protected]> wrote:
>
> On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <[email protected]> wrote:
> > >
> > > The hard-coded 16 is used in various bpf progs. These progs get task
> > > comm either via bpf_get_current_comm() or prctl() or
> > > bpf_core_read_str(), all of which can work well even if the task comm size
> > > is changed.
> > > Below is the detailed information,
> > >
> > > bpf_get_current_comm:
> > > progs/test_ringbuf.c
> > > progs/test_ringbuf_multi.c
> > >
> > > prctl:
> > > prog_tests/test_overhead.c
> > > prog_tests/trampoline_count.c
> > >
> > > bpf_core_read_str:
> > > progs/test_core_reloc_kernel.c
> > > progs/test_sk_storage_tracing.c
> > >
> > > We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it
> > > more grepable.
> > >
> > > Signed-off-by: Yafang Shao <[email protected]>
> > > Cc: Mathieu Desnoyers <[email protected]>
> > > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > > Cc: Peter Zijlstra <[email protected]>
> > > Cc: Steven Rostedt <[email protected]>
> > > Cc: Kees Cook <[email protected]>
> > > Cc: Al Viro <[email protected]>
> > > Cc: Petr Mladek <[email protected]>
> > > ---
> > > tools/testing/selftests/bpf/Makefile | 2 +-
> > > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++-
> > > tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++-
> > > .../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++-
> > > tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++-
> > > tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++-
> > > tools/testing/selftests/bpf/progs/profiler.h | 7 ++++---
> > > tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++----
> > > tools/testing/selftests/bpf/progs/pyperf.h | 4 ++--
> > > tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++---
> > > .../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++-
> > > tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++-
> > > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++-
> > > .../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++--
> > > tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++---
> > > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++--
> > > tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++--
> > > 17 files changed, 41 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 799b88152e9e..5e72d783d3fe 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
> > >
> > > CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
> > > BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
> > > - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
> > > + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \
> >
> > please don't add new include paths unnecessarily. See my comment on
> > another patch, if you add those new constants as enums, they will be
> > automatically available in vmlinux BTF and thus in auto-generated
> > vmlinux.h header (for those programs using it).
>
> Yes, after converting it to enum, the BPF programs can get it from the
> generated vmlinux.h.
>
> > For others, I'd just
> > leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate.
> >
>
> It seems not all the BPF programs can include the vmlinux.h.
> What we really care about here is the copy of task comm should be with
> a nul terminator, if we can assure it, then the size used by the BPF
> is not important.
> I have checked the copy of task comm in all these BPF programs one by
> one, and replaced the unsafe bpf_probe_read_kernel() with
> bpf_probe_read_kernel_str(), after that change, I think we can leave
> hard-coded 16 for the progs which can't include vmlinux.h.

SGTM, thanks.

>
> > > -I$(abspath $(OUTPUT)/../usr/include)
> > >
> > > CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > > index 4706cee84360..ac82d57c09dc 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > > @@ -12,6 +12,7 @@
> > > #include <sys/sysinfo.h>
> > > #include <linux/perf_event.h>
> > > #include <linux/ring_buffer.h>
> > > +#include <linux/sched/task.h>
> > > #include "test_ringbuf.lskel.h"
> > >
> > > #define EDONE 7777
> > > @@ -22,7 +23,7 @@ struct sample {
> > > int pid;
> > > int seq;
> > > long value;
> > > - char comm[16];
> > > + char comm[TASK_COMM_LEN_16];
> >
> > how much value is in this "grep-ability", really? I'm not convinced
> > all this code churn is justified.
> >
> > > };
> > >
> > > static int sample_cnt;
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > index 167cd8a2edfd..f0748305ffd6 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > @@ -2,6 +2,7 @@
> > > #define _GNU_SOURCE
> > > #include <test_progs.h>
> > > #include <sys/epoll.h>
> > > +#include <linux/sched/task.h>
> > > #include "test_ringbuf_multi.skel.h"
> > >
> > > static int duration = 0;
> >
> > [...]
>
>
> --
> Thanks
> Yafang