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

[Andrii Nakryiko]

On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>
> ---
>  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;

[...]