This is the followup work of task comm cleanups[1].
In this phase, the hard-coded 16 is replaced by TASK_COMM_LEN_16 to make
it grepable. The difference between this two marcos is:
- TASK_COMM_LEN
The size should be same with the TASK_COMM_LEN defined in linux/sched.h.
- TASK_COMM_LEN_16
The size must be a fixed-size 16 no matter what TASK_COMM_LEN is. It may
be exposed to userspace so we can't change it.
1. https://lore.kernel.org/lkml/[email protected]/
Changes since v1:
- use TASK_COMM_LEN_16 instead of TASK_COMM_LEN in patch #3 (Steven)
- avoid changing samples/bpf and bpf/progs (Alexei)
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
Yafang Shao (3):
elfcore: replace old hard-code 16 with TASK_COMM_LEN_16
cn_proc: replaced old hard-coded 16 with TASK_COMM_LEN_16
tools/perf: replace old hard-coded 16 with TASK_COMM_LEN_16
include/linux/elfcore-compat.h | 8 ++------
include/linux/elfcore.h | 9 ++-------
include/linux/sched.h | 5 +++++
include/uapi/linux/cn_proc.h | 4 +++-
tools/perf/tests/evsel-tp-sched.c | 8 +++++---
5 files changed, 17 insertions(+), 17 deletions(-)
--
2.17.1
A new macro TASK_COMM_LEN_16 is introduced for the old hard-coded 16 to
make it more grepable. As explained above this marco, the difference
between TASK_COMM_LEN and TASK_COMM_LEN_16 is that TASK_COMM_LEN_16 must
be a fixed size 16 and can't be changed.
Suggested-by: Steven Rostedt (VMware) <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
include/linux/elfcore-compat.h | 8 ++------
include/linux/elfcore.h | 9 ++-------
include/linux/sched.h | 5 +++++
3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
index 54feb64e9b5d..69fa1a728964 100644
--- a/include/linux/elfcore-compat.h
+++ b/include/linux/elfcore-compat.h
@@ -5,6 +5,7 @@
#include <linux/elf.h>
#include <linux/elfcore.h>
#include <linux/compat.h>
+#include <linux/sched.h>
/*
* Make sure these layouts match the linux/elfcore.h native definitions.
@@ -43,12 +44,7 @@ struct compat_elf_prpsinfo
__compat_uid_t pr_uid;
__compat_gid_t pr_gid;
compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
- /*
- * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be
- * changed as it is exposed to userspace. We'd better make it hard-coded
- * here.
- */
- char pr_fname[16];
+ char pr_fname[TASK_COMM_LEN_16];
char pr_psargs[ELF_PRARGSZ];
};
diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
index 746e081879a5..d3bb4bd3c985 100644
--- a/include/linux/elfcore.h
+++ b/include/linux/elfcore.h
@@ -65,13 +65,8 @@ struct elf_prpsinfo
__kernel_gid_t pr_gid;
pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
/* Lots missing */
- /*
- * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be
- * changed as it is exposed to userspace. We'd better make it hard-coded
- * here.
- */
- char pr_fname[16]; /* filename of executable */
- char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
+ char pr_fname[TASK_COMM_LEN_16]; /* filename of executable */
+ char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
};
static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c79bd7ee6029..8d963a50a2a8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -279,6 +279,11 @@ struct task_group;
* BPF programs.
*/
enum {
+ /*
+ * For the old hard-coded 16, which is exposed to userspace and can't
+ * be changed.
+ */
+ TASK_COMM_LEN_16 = 16,
TASK_COMM_LEN = 16,
};
--
2.17.1
evsel-tp-sched will verify the task comm len in sched:sched_switch
and sched:sched_wakeup tracepoints. In order to make it grepable, we'd
better replace the hard-coded 16 with TASK_COMM_LEN_16.
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
tools/perf/tests/evsel-tp-sched.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index cf4da3d748c2..8be44b8e2b9c 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -5,6 +5,8 @@
#include "tests.h"
#include "debug.h"
+#define TASK_COMM_LEN_16 16
+
static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
{
struct tep_format_field *field = evsel__field(evsel, name);
@@ -43,7 +45,7 @@ static int test__perf_evsel__tp_sched_test(struct test_suite *test __maybe_unuse
return -1;
}
- if (evsel__test_field(evsel, "prev_comm", 16, false))
+ if (evsel__test_field(evsel, "prev_comm", TASK_COMM_LEN_16, false))
ret = -1;
if (evsel__test_field(evsel, "prev_pid", 4, true))
@@ -55,7 +57,7 @@ static int test__perf_evsel__tp_sched_test(struct test_suite *test __maybe_unuse
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(evsel, "next_comm", TASK_COMM_LEN_16, false))
ret = -1;
if (evsel__test_field(evsel, "next_pid", 4, true))
@@ -73,7 +75,7 @@ static int test__perf_evsel__tp_sched_test(struct test_suite *test __maybe_unuse
return -1;
}
- if (evsel__test_field(evsel, "comm", 16, false))
+ if (evsel__test_field(evsel, "comm", TASK_COMM_LEN_16, false))
ret = -1;
if (evsel__test_field(evsel, "pid", 4, true))
--
2.17.1
This TASK_COMM_LEN_16 has the same meaning with the macro defined in
linux/sched.h, but we can't include linux/sched.h in a UAPI header, so
we should specifically define it in the cn_proc.h.
Signed-off-by: Yafang Shao <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
include/uapi/linux/cn_proc.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
index db210625cee8..6dcccaed383f 100644
--- a/include/uapi/linux/cn_proc.h
+++ b/include/uapi/linux/cn_proc.h
@@ -21,6 +21,8 @@
#include <linux/types.h>
+#define TASK_COMM_LEN_16 16
+
/*
* Userspace sends this enum to register with the kernel that it is listening
* for events on the connector.
@@ -110,7 +112,7 @@ struct proc_event {
struct comm_proc_event {
__kernel_pid_t process_pid;
__kernel_pid_t process_tgid;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
} comm;
struct coredump_proc_event {
--
2.17.1
On Sat, Dec 11, 2021 at 06:39:48AM +0000, Yafang Shao wrote:
> This TASK_COMM_LEN_16 has the same meaning with the macro defined in
> linux/sched.h, but we can't include linux/sched.h in a UAPI header, so
> we should specifically define it in the cn_proc.h.
[...]
> index db210625cee8..6dcccaed383f 100644
> --- a/include/uapi/linux/cn_proc.h
> +++ b/include/uapi/linux/cn_proc.h
> @@ -21,6 +21,8 @@
>
> #include <linux/types.h>
>
> +#define TASK_COMM_LEN_16 16
Hi,
Since this is added to UAPI header, maybe you could make it a single
instance also used elsewhere? Even though this is constant and not
going to change I don't really like multiplying the sources of truth.
Best Regards
Micha? Miros?aw
On Sun, Dec 12, 2021 at 2:10 PM Michal Miroslaw <[email protected]> wrote:
>
> On Sat, Dec 11, 2021 at 06:39:48AM +0000, Yafang Shao wrote:
> > This TASK_COMM_LEN_16 has the same meaning with the macro defined in
> > linux/sched.h, but we can't include linux/sched.h in a UAPI header, so
> > we should specifically define it in the cn_proc.h.
> [...]
> > index db210625cee8..6dcccaed383f 100644
> > --- a/include/uapi/linux/cn_proc.h
> > +++ b/include/uapi/linux/cn_proc.h
> > @@ -21,6 +21,8 @@
> >
> > #include <linux/types.h>
> >
> > +#define TASK_COMM_LEN_16 16
>
> Hi,
>
> Since this is added to UAPI header, maybe you could make it a single
> instance also used elsewhere? Even though this is constant and not
> going to change I don't really like multiplying the sources of truth.
>
Hmm, what about defining it in include/uapi/linux/sched.h ?
Then include "sched.h" in cn_proc.h
And we also define it in tools/include/uapi/linux/sched.h for the
usage in tools.
--
Thanks
Yafang
On 11.12.21 07:39, Yafang Shao wrote:
> A new macro TASK_COMM_LEN_16 is introduced for the old hard-coded 16 to
> make it more grepable. As explained above this marco, the difference
> between TASK_COMM_LEN and TASK_COMM_LEN_16 is that TASK_COMM_LEN_16 must
> be a fixed size 16 and can't be changed.
>
> Suggested-by: Steven Rostedt (VMware) <[email protected]>
> Signed-off-by: Yafang Shao <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> include/linux/elfcore-compat.h | 8 ++------
> include/linux/elfcore.h | 9 ++-------
> include/linux/sched.h | 5 +++++
> 3 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
> index 54feb64e9b5d..69fa1a728964 100644
> --- a/include/linux/elfcore-compat.h
> +++ b/include/linux/elfcore-compat.h
> @@ -5,6 +5,7 @@
> #include <linux/elf.h>
> #include <linux/elfcore.h>
> #include <linux/compat.h>
> +#include <linux/sched.h>
>
> /*
> * Make sure these layouts match the linux/elfcore.h native definitions.
> @@ -43,12 +44,7 @@ struct compat_elf_prpsinfo
> __compat_uid_t pr_uid;
> __compat_gid_t pr_gid;
> compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> - /*
> - * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be
> - * changed as it is exposed to userspace. We'd better make it hard-coded
> - * here.
> - */
> - char pr_fname[16];
> + char pr_fname[TASK_COMM_LEN_16];
> char pr_psargs[ELF_PRARGSZ];
> };
>
> diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
> index 746e081879a5..d3bb4bd3c985 100644
> --- a/include/linux/elfcore.h
> +++ b/include/linux/elfcore.h
> @@ -65,13 +65,8 @@ struct elf_prpsinfo
> __kernel_gid_t pr_gid;
> pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> /* Lots missing */
> - /*
> - * The hard-coded 16 is derived from TASK_COMM_LEN, but it can't be
> - * changed as it is exposed to userspace. We'd better make it hard-coded
> - * here.
> - */
> - char pr_fname[16]; /* filename of executable */
> - char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
> + char pr_fname[TASK_COMM_LEN_16]; /* filename of executable */
> + char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
> };
>
> static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c79bd7ee6029..8d963a50a2a8 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -279,6 +279,11 @@ struct task_group;
> * BPF programs.
> */
> enum {
> + /*
> + * For the old hard-coded 16, which is exposed to userspace and can't
> + * be changed.
> + */
> + TASK_COMM_LEN_16 = 16,
> TASK_COMM_LEN = 16,
> };
>
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb