* [PATCH v5 11/15] tools/bpf/bpftool: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 3:45 [PATCH v5 10/15] tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 22:38 ` Andrii Nakryiko
2021-10-21 3:46 ` [PATCH v5 12/15] tools/perf/test: make perf test adopt to task comm size change Yafang Shao
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
Use TASK_COMM_LEN_16 instead of hard-coded 16 to make it more grepable.
It uses bpf_probe_read_kernel() to get task comm, which may return a
string without nul terminator. We should use bpf_probe_read_kernel_str()
instead.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/bpf/bpftool/Makefile | 1 +
tools/bpf/bpftool/main.h | 3 ++-
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
tools/bpf/bpftool/skeleton/pid_iter.h | 4 +++-
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index d73232be1e99..33fbde84993c 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -164,6 +164,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF)
$(QUIET_CLANG)$(CLANG) \
-I$(if $(OUTPUT),$(OUTPUT),.) \
-I$(srctree)/tools/include/uapi/ \
+ -I$(srctree)/tools/include/ \
-I$(LIBBPF_PATH) \
-I$(srctree)/tools/lib \
-g -O2 -Wall -target bpf -c $< -o $@ && $(LLVM_STRIP) -g $@
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 90caa42aac4c..5efa27188f68 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -12,6 +12,7 @@
#include <linux/compiler.h>
#include <linux/kernel.h>
#include <linux/hashtable.h>
+#include <linux/sched/task.h>
#include <tools/libc_compat.h>
#include <bpf/libbpf.h>
@@ -124,7 +125,7 @@ struct obj_refs_table {
struct obj_ref {
int pid;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};
struct obj_refs {
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index d9b420972934..f70702fcb224 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -71,8 +71,8 @@ int iter(struct bpf_iter__task_file *ctx)
e.pid = task->tgid;
e.id = get_obj_id(file->private_data, obj_type);
- bpf_probe_read_kernel(&e.comm, sizeof(e.comm),
- task->group_leader->comm);
+ bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
+ task->group_leader->comm);
bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
return 0;
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
index 5692cf257adb..675b2916567e 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.h
+++ b/tools/bpf/bpftool/skeleton/pid_iter.h
@@ -3,10 +3,12 @@
#ifndef __PID_ITER_H
#define __PID_ITER_H
+#include <linux/sched/task.h>
+
struct pid_iter_entry {
__u32 id;
int pid;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
};
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v5 11/15] tools/bpf/bpftool: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 3:45 ` [PATCH v5 11/15] tools/bpf/bpftool: " Yafang Shao
@ 2021-10-21 22:38 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-10-21 22:38 UTC (permalink / raw)
To: Yafang Shao
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, qiang.zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, juri.lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, oliver.sang, kbuild test robot
On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Use TASK_COMM_LEN_16 instead of hard-coded 16 to make it more grepable.
> It uses bpf_probe_read_kernel() to get task comm, which may return a
> string without nul terminator. We should use bpf_probe_read_kernel_str()
> instead.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
> tools/bpf/bpftool/Makefile | 1 +
> tools/bpf/bpftool/main.h | 3 ++-
> tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
> tools/bpf/bpftool/skeleton/pid_iter.h | 4 +++-
> 4 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index d73232be1e99..33fbde84993c 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -164,6 +164,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF)
> $(QUIET_CLANG)$(CLANG) \
> -I$(if $(OUTPUT),$(OUTPUT),.) \
> -I$(srctree)/tools/include/uapi/ \
> + -I$(srctree)/tools/include/ \
bpftool shouldn't rely on internal kernel headers for compilation. If
you want to have TASK_COMM_LEN_16 constant for grep-ability, just
#define it where appropriate
> -I$(LIBBPF_PATH) \
> -I$(srctree)/tools/lib \
> -g -O2 -Wall -target bpf -c $< -o $@ && $(LLVM_STRIP) -g $@
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 90caa42aac4c..5efa27188f68 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -12,6 +12,7 @@
> #include <linux/compiler.h>
> #include <linux/kernel.h>
> #include <linux/hashtable.h>
> +#include <linux/sched/task.h>
> #include <tools/libc_compat.h>
>
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 12/15] tools/perf/test: make perf test adopt to task comm size change
2021-10-21 3:45 [PATCH v5 10/15] tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
2021-10-21 3:45 ` [PATCH v5 11/15] tools/bpf/bpftool: " Yafang Shao
@ 2021-10-21 3:46 ` Yafang Shao
2021-10-21 3:46 ` [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2021-10-21 3:46 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
kernel test robot reported a perf-test failure after I extended task comm
size from 16 to 24. The failure as follows,
2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
15: Parse sched tracepoints fields : FAILED!
The reason is perf-test requires a fixed-size task comm. If we extend
task comm size to 24, it will not equil with the required size 16 in perf
test.
After some analyzation, I found perf itself can adopt to the size
change, for example, below is the output of perf-sched after I extend
comm size to 24 -
task 614 ( kthreadd: 84), nr_events: 1
task 615 ( systemd: 843), nr_events: 1
task 616 ( networkd-dispat: 1026), nr_events: 1
task 617 ( systemd: 846), nr_events: 1
$ cat /proc/843/comm
networkd-dispatcher
The task comm can be displayed correctly as expected.
Now that task comm can have a different size, then the test should accept
the two sizes as possible and pass if it is 16 or 24. In order to do
that, a new macro TASK_COMM_LEN_24 is introduced.
After this patch, the perf-test succeeds no matter task comm is 16 or
24 -
15: Parse sched tracepoints fields : Ok
This patch is a preparation for the followup patch.
Reported-by: kernel test robot <oliver.sang@intel.com>
Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/include/linux/sched/task.h | 1 +
tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/tools/include/linux/sched/task.h b/tools/include/linux/sched/task.h
index 7657dd3e0e02..da49ac983ac6 100644
--- a/tools/include/linux/sched/task.h
+++ b/tools/include/linux/sched/task.h
@@ -2,5 +2,6 @@
#define _TOOLS_PERF_LINUX_SCHED_TASK_H
#define TASK_COMM_LEN_16 16
+#define TASK_COMM_LEN_24 24
#endif /* _TOOLS_PERF_LINUX_SCHED_TASK_H */
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index f9e34bd26cf3..cf4e0472e29e 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -1,11 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/err.h>
+#include <linux/sched/task.h>
#include <traceevent/event-parse.h>
#include "evsel.h"
#include "tests.h"
#include "debug.h"
-static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
+static int evsel__test_field_alt(struct evsel *evsel, const char *name,
+ int size, int alternate_size, bool should_be_signed)
{
struct tep_format_field *field = evsel__field(evsel, name);
int is_signed;
@@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, bo
ret = -1;
}
- if (field->size != size) {
- pr_debug("%s: \"%s\" size (%d) should be %d!\n",
+ if (field->size != size && field->size != alternate_size) {
+ pr_debug("%s: \"%s\" size (%d) should be %d",
evsel->name, name, field->size, size);
+ if (alternate_size > 0)
+ pr_debug(" or %d", alternate_size);
+ pr_debug("!\n");
ret = -1;
}
return ret;
}
+static int evsel__test_field(struct evsel *evsel, const char *name,
+ int size, bool should_be_signed)
+{
+ return evsel__test_field_alt(evsel, name, size, -1, should_be_signed);
+}
+
int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtest __maybe_unused)
{
struct evsel *evsel = evsel__newtp("sched", "sched_switch");
@@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
return -1;
}
- if (evsel__test_field(evsel, "prev_comm", 16, false))
+ if (evsel__test_field_alt(evsel, "prev_comm", TASK_COMM_LEN_16,
+ TASK_COMM_LEN_24, false))
ret = -1;
if (evsel__test_field(evsel, "prev_pid", 4, true))
@@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
if (evsel__test_field(evsel, "prev_state", sizeof(long), true))
ret = -1;
- if (evsel__test_field(evsel, "next_comm", 16, false))
+ if (evsel__test_field_alt(evsel, "next_comm", TASK_COMM_LEN_16,
+ TASK_COMM_LEN_24, false))
ret = -1;
if (evsel__test_field(evsel, "next_pid", 4, true))
@@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
return -1;
}
- if (evsel__test_field(evsel, "comm", 16, false))
+ if (evsel__test_field_alt(evsel, "comm", TASK_COMM_LEN_16,
+ TASK_COMM_LEN_24, false))
ret = -1;
if (evsel__test_field(evsel, "pid", 4, true))
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 3:45 [PATCH v5 10/15] tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
2021-10-21 3:45 ` [PATCH v5 11/15] tools/bpf/bpftool: " Yafang Shao
2021-10-21 3:46 ` [PATCH v5 12/15] tools/perf/test: make perf test adopt to task comm size change Yafang Shao
@ 2021-10-21 3:46 ` Yafang Shao
2021-10-21 22:44 ` Andrii Nakryiko
2021-10-21 3:46 ` [PATCH v5 14/15] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
2021-10-21 3:46 ` [PATCH v5 15/15] kernel/kthread: show a warning if kthread's comm is truncated Yafang Shao
4 siblings, 1 reply; 10+ messages in thread
From: Yafang Shao @ 2021-10-21 3:46 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
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 <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 3:46 ` [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
@ 2021-10-21 22:44 ` Andrii Nakryiko
2021-10-22 6:36 ` Yafang Shao
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2021-10-21 22:44 UTC (permalink / raw)
To: Yafang Shao
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, qiang.zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, juri.lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, oliver.sang, kbuild test robot
On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@gmail.com> 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 <laoar.shao@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
> 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;
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 22:44 ` Andrii Nakryiko
@ 2021-10-22 6:36 ` Yafang Shao
2021-10-22 23:41 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Yafang Shao @ 2021-10-22 6:36 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, Qiang Zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, Juri Lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, kernel test robot, kbuild test robot
On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@gmail.com> 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 <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-22 6:36 ` Yafang Shao
@ 2021-10-22 23:41 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-10-22 23:41 UTC (permalink / raw)
To: Yafang Shao
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, Qiang Zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, Juri Lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, kernel test robot, kbuild test robot
On Thu, Oct 21, 2021 at 11:36 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@gmail.com> 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 <laoar.shao@gmail.com>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Petr Mladek <pmladek@suse.com>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 14/15] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL
2021-10-21 3:45 [PATCH v5 10/15] tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
` (2 preceding siblings ...)
2021-10-21 3:46 ` [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
@ 2021-10-21 3:46 ` Yafang Shao
2021-10-21 3:46 ` [PATCH v5 15/15] kernel/kthread: show a warning if kthread's comm is truncated Yafang Shao
4 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2021-10-21 3:46 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,
jbd2/nvme0n1p2-
xfs-reclaim/sdf
We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:
rcu_tasks_kthre
rcu_tasks_rude_
rcu_tasks_trace
poll_mpt3sas0_s
ext4-rsv-conver
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
audit_send_repl
ecryptfs-kthrea
vfio-irqfd-clea
jbd2/nvme0n1p2-
...
We should improve this problem fundamentally.
This patch extends the size of task comm to 24 bytes, which is the
same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.
After this patchset, the truncated kthreads listed above will be
displayed as:
rcu_tasks_kthread
rcu_tasks_rude_kthread
rcu_tasks_trace_kthread
poll_mpt3sas0_statu
ext4-rsv-conversion
xfs-reclaim/sdf1
xfs-blockgc/sdf1
xfs-inodegc/sdf1
audit_send_reply
ecryptfs-kthread
vfio-irqfd-cleanup
jbd2/nvme0n1p2-8
Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 62d5b30d310c..fcb4bc97f95c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -274,10 +274,17 @@ struct task_group;
#define get_current_state() READ_ONCE(current->__state)
-/* To replace the old hard-coded 16 */
-#define TASK_COMM_LEN_16 16
+/* Also to replace the old hard-coded 16 */
+#define TASK_COMM_LEN_16 16
+#define TASK_COMM_LEN_24 24
+
+
/* Task command name length: */
-#define TASK_COMM_LEN 16
+#if CONFIG_BASE_SMALL
+#define TASK_COMM_LEN TASK_COMM_LEN_16
+#else
+#define TASK_COMM_LEN TASK_COMM_LEN_24
+#endif
extern void scheduler_tick(void);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v5 15/15] kernel/kthread: show a warning if kthread's comm is truncated
2021-10-21 3:45 [PATCH v5 10/15] tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 Yafang Shao
` (3 preceding siblings ...)
2021-10-21 3:46 ` [PATCH v5 14/15] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
@ 2021-10-21 3:46 ` Yafang Shao
4 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2021-10-21 3:46 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
Show a warning if task comm is truncated. Below is the result
of my test case:
truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
kernel/kthread.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..46b924c92078 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -399,12 +399,17 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
if (!IS_ERR(task)) {
static const struct sched_param param = { .sched_priority = 0 };
char name[TASK_COMM_LEN];
+ int len;
/*
* task is already visible to other tasks, so updating
* COMM must be protected.
*/
- vsnprintf(name, sizeof(name), namefmt, args);
+ len = vsnprintf(name, sizeof(name), namefmt, args);
+ if (len >= TASK_COMM_LEN) {
+ pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
+ name, task->pid, len - TASK_COMM_LEN + 1);
+ }
set_task_comm(task, name);
/*
* root may have changed our (kthreadd's) priority or CPU mask.
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread