* [PATCH v2 0/7] task comm cleanups @ 2021-11-20 11:27 Yafang Shao 2021-11-20 11:27 ` [PATCH v2 1/7] fs/exec: replace strlcpy with strscpy_pad in __set_task_comm Yafang Shao ` (6 more replies) 0 siblings, 7 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, David Hildenbrand, Al Viro, Kees Cook, Petr Mladek This patchset is part of the patchset "extend task comm from 16 to 24"[1]. Now we have different opinion that dynamically allocates memory to store kthread's long name into a separate pointer, so I decide to take the useful cleanups apart from the original patchset and send it separately[2]. These useful cleanups can make the usage around task comm less error-prone. Furthermore, it will be useful if we want to extend task comm in the future. [1]. https://lore.kernel.org/lkml/20211101060419.4682-1-laoar.shao@gmail.com/ [2]. https://lore.kernel.org/lkml/CALOAHbAx55AUo3bm8ZepZSZnw7A08cvKPdPyNTf=E_tPqmw5hw@mail.gmail.com/ Changes since v1: - improve the subject and description - add comment to explain the hard-coded 16 in patch #4 Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> Yafang Shao (7): fs/exec: replace strlcpy with strscpy_pad in __set_task_comm fs/exec: replace strncpy with strscpy_pad in __get_task_comm drivers/infiniband: replace open-coded string copy with get_task_comm fs/binfmt_elf: replace open-coded string copy with get_task_comm samples/bpf/test_overhead_kprobe_kern: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm tools/bpf/bpftool/skeleton: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN drivers/infiniband/hw/qib/qib.h | 2 +- drivers/infiniband/hw/qib/qib_file_ops.c | 2 +- fs/binfmt_elf.c | 2 +- fs/exec.c | 5 +++-- include/linux/elfcore-compat.h | 5 +++++ include/linux/elfcore.h | 5 +++++ include/linux/sched.h | 9 +++++++-- samples/bpf/offwaketime_kern.c | 4 ++-- samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++----- samples/bpf/test_overhead_tp_kern.c | 5 +++-- tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++-- .../testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- 13 files changed, 42 insertions(+), 24 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 1/7] fs/exec: replace strlcpy with strscpy_pad in __set_task_comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 2/7] fs/exec: replace strncpy with strscpy_pad in __get_task_comm Yafang Shao ` (5 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Petr Mladek strlcpy() can trigger out-of-bound reads on the source string[1], we'd better use strscpy() instead. To make it be robust against full tsk->comm copies that got noticed in other places, we should make sure it's zero padded. [1] https://github.com/KSPP/linux/issues/89 Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index 537d92c41105..51d3cb4e3cdf 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1222,7 +1222,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) { task_lock(tsk); trace_task_rename(tsk, buf); - strlcpy(tsk->comm, buf, sizeof(tsk->comm)); + strscpy_pad(tsk->comm, buf, sizeof(tsk->comm)); task_unlock(tsk); perf_event_comm(tsk, exec); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 2/7] fs/exec: replace strncpy with strscpy_pad in __get_task_comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao 2021-11-20 11:27 ` [PATCH v2 1/7] fs/exec: replace strlcpy with strscpy_pad in __set_task_comm Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 3/7] drivers/infiniband: replace open-coded string copy with get_task_comm Yafang Shao ` (4 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Kees Cook, Steven Rostedt, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Petr Mladek If the dest buffer size is smaller than sizeof(tsk->comm), the buffer will be without null ternimator, that may cause problem. Using strscpy_pad() instead of strncpy() in __get_task_comm() can make the string always nul ternimated and zero padded. Suggested-by: Kees Cook <keescook@chromium.org> Suggested-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- fs/exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index 51d3cb4e3cdf..fa142638b191 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1207,7 +1207,8 @@ static int unshare_sighand(struct task_struct *me) char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) { task_lock(tsk); - strncpy(buf, tsk->comm, buf_size); + /* Always NUL terminated and zero-padded */ + strscpy_pad(buf, tsk->comm, buf_size); task_unlock(tsk); return buf; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 3/7] drivers/infiniband: replace open-coded string copy with get_task_comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao 2021-11-20 11:27 ` [PATCH v2 1/7] fs/exec: replace strlcpy with strscpy_pad in __set_task_comm Yafang Shao 2021-11-20 11:27 ` [PATCH v2 2/7] fs/exec: replace strncpy with strscpy_pad in __get_task_comm Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 4/7] fs/binfmt_elf: " Yafang Shao ` (3 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Dennis Dalessandro, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek We'd better use the helper get_task_comm() rather than the open-coded strlcpy() to get task comm. As the comment above the hard-coded 16, we can replace it with TASK_COMM_LEN. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- drivers/infiniband/hw/qib/qib.h | 2 +- drivers/infiniband/hw/qib/qib_file_ops.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h index 9363bccfc6e7..a8e1c30c370f 100644 --- a/drivers/infiniband/hw/qib/qib.h +++ b/drivers/infiniband/hw/qib/qib.h @@ -196,7 +196,7 @@ struct qib_ctxtdata { pid_t pid; pid_t subpid[QLOGIC_IB_MAX_SUBCTXT]; /* same size as task_struct .comm[], command that opened context */ - char comm[16]; + char comm[TASK_COMM_LEN]; /* pkeys set by this use of this ctxt */ u16 pkeys[4]; /* so file ops can get at unit */ diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c index 63854f4b6524..aa290928cf96 100644 --- a/drivers/infiniband/hw/qib/qib_file_ops.c +++ b/drivers/infiniband/hw/qib/qib_file_ops.c @@ -1321,7 +1321,7 @@ static int setup_ctxt(struct qib_pportdata *ppd, int ctxt, rcd->tid_pg_list = ptmp; rcd->pid = current->pid; init_waitqueue_head(&dd->rcd[ctxt]->wait); - strlcpy(rcd->comm, current->comm, sizeof(rcd->comm)); + get_task_comm(rcd->comm, current); ctxt_fp(fp) = rcd; qib_stats.sps_ctxts++; dd->freectxts--; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 4/7] fs/binfmt_elf: replace open-coded string copy with get_task_comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao ` (2 preceding siblings ...) 2021-11-20 11:27 ` [PATCH v2 3/7] drivers/infiniband: replace open-coded string copy with get_task_comm Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-29 16:01 ` Steven Rostedt 2021-11-20 11:27 ` [PATCH v2 5/7] samples/bpf/test_overhead_kprobe_kern: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm Yafang Shao ` (2 subsequent siblings) 6 siblings, 1 reply; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Petr Mladek It is better to use get_task_comm() instead of the open coded string copy as we do in other places. struct elf_prpsinfo is used to dump the task information in userspace coredump or kernel vmcore. Below is the verification of vmcore, crash> ps PID PPID CPU TASK ST %MEM VSZ RSS COMM 0 0 0 ffffffff9d21a940 RU 0.0 0 0 [swapper/0] > 0 0 1 ffffa09e40f85e80 RU 0.0 0 0 [swapper/1] > 0 0 2 ffffa09e40f81f80 RU 0.0 0 0 [swapper/2] > 0 0 3 ffffa09e40f83f00 RU 0.0 0 0 [swapper/3] > 0 0 4 ffffa09e40f80000 RU 0.0 0 0 [swapper/4] > 0 0 5 ffffa09e40f89f80 RU 0.0 0 0 [swapper/5] 0 0 6 ffffa09e40f8bf00 RU 0.0 0 0 [swapper/6] > 0 0 7 ffffa09e40f88000 RU 0.0 0 0 [swapper/7] > 0 0 8 ffffa09e40f8de80 RU 0.0 0 0 [swapper/8] > 0 0 9 ffffa09e40f95e80 RU 0.0 0 0 [swapper/9] > 0 0 10 ffffa09e40f91f80 RU 0.0 0 0 [swapper/10] > 0 0 11 ffffa09e40f93f00 RU 0.0 0 0 [swapper/11] > 0 0 12 ffffa09e40f90000 RU 0.0 0 0 [swapper/12] > 0 0 13 ffffa09e40f9bf00 RU 0.0 0 0 [swapper/13] > 0 0 14 ffffa09e40f98000 RU 0.0 0 0 [swapper/14] > 0 0 15 ffffa09e40f9de80 RU 0.0 0 0 [swapper/15] It works well as expected. Some comments are added to explain why we use the hard-coded 16. Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- fs/binfmt_elf.c | 2 +- include/linux/elfcore-compat.h | 5 +++++ include/linux/elfcore.h | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index f8c7f26f1fbb..b9a33cc34d6b 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1585,7 +1585,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p, SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid)); SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid)); rcu_read_unlock(); - strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname)); + get_task_comm(psinfo->pr_fname, p); return 0; } diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h index e272c3d452ce..54feb64e9b5d 100644 --- a/include/linux/elfcore-compat.h +++ b/include/linux/elfcore-compat.h @@ -43,6 +43,11 @@ 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_psargs[ELF_PRARGSZ]; }; diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h index 957ebec35aad..746e081879a5 100644 --- a/include/linux/elfcore.h +++ b/include/linux/elfcore.h @@ -65,6 +65,11 @@ 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 */ }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/7] fs/binfmt_elf: replace open-coded string copy with get_task_comm 2021-11-20 11:27 ` [PATCH v2 4/7] fs/binfmt_elf: " Yafang Shao @ 2021-11-29 16:01 ` Steven Rostedt 2021-11-30 3:01 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2021-11-29 16:01 UTC (permalink / raw) To: Yafang Shao Cc: akpm, netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Petr Mladek On Sat, 20 Nov 2021 11:27:35 +0000 Yafang Shao <laoar.shao@gmail.com> wrote: > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h > index e272c3d452ce..54feb64e9b5d 100644 > --- a/include/linux/elfcore-compat.h > +++ b/include/linux/elfcore-compat.h > @@ -43,6 +43,11 @@ 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. Didn't I once suggest having a macro called something like: TASK_COMM_LEN_16 ? https://lore.kernel.org/all/20211014221409.5da58a42@oasis.local.home/ -- Steve > + */ > char pr_fname[16]; > char pr_psargs[ELF_PRARGSZ]; > }; > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h > index 957ebec35aad..746e081879a5 100644 > --- a/include/linux/elfcore.h > +++ b/include/linux/elfcore.h > @@ -65,6 +65,11 @@ 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 */ > }; ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/7] fs/binfmt_elf: replace open-coded string copy with get_task_comm 2021-11-29 16:01 ` Steven Rostedt @ 2021-11-30 3:01 ` Yafang Shao 2021-11-30 14:22 ` Steven Rostedt 0 siblings, 1 reply; 43+ messages in thread From: Yafang Shao @ 2021-11-30 3:01 UTC (permalink / raw) To: Steven Rostedt Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Petr Mladek On Tue, Nov 30, 2021 at 12:01 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Sat, 20 Nov 2021 11:27:35 +0000 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h > > index e272c3d452ce..54feb64e9b5d 100644 > > --- a/include/linux/elfcore-compat.h > > +++ b/include/linux/elfcore-compat.h > > @@ -43,6 +43,11 @@ 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. > > Didn't I once suggest having a macro called something like: > > TASK_COMM_LEN_16 ? > > > https://lore.kernel.org/all/20211014221409.5da58a42@oasis.local.home/ Hi Steven, TASK_COMM_LEN_16 is a good idea, but not all hard-coded 16 can be replaced by this macro (which is defined in include/sched.h). For example, the comm[16] in include/uapi/linux/cn_proc.h can't be replaced as it is a uapi header which can't include linux/sched.h. That's why I prefer to keep the hard-coded 16 as-is. There are three options, - option 1 comment on all the hard-coded 16 to explain why it is hard-coded - option 2 replace the hard-coded 16 that can be replaced and comment on the others which can't be replaced. - option 3 replace the hard-coded 16 that can be replaced and specifically define TASK_COMM_LEN_16 in other files which can't include linux/sched.h. Which one do you prefer ? > > > > + */ > > char pr_fname[16]; > > char pr_psargs[ELF_PRARGSZ]; > > }; > > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h > > index 957ebec35aad..746e081879a5 100644 > > --- a/include/linux/elfcore.h > > +++ b/include/linux/elfcore.h > > @@ -65,6 +65,11 @@ 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 */ > > }; -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/7] fs/binfmt_elf: replace open-coded string copy with get_task_comm 2021-11-30 3:01 ` Yafang Shao @ 2021-11-30 14:22 ` Steven Rostedt 2021-11-30 15:53 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2021-11-30 14:22 UTC (permalink / raw) To: Yafang Shao Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Petr Mladek On Tue, 30 Nov 2021 11:01:27 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > There are three options, > - option 1 > comment on all the hard-coded 16 to explain why it is hard-coded > - option 2 > replace the hard-coded 16 that can be replaced and comment on the > others which can't be replaced. > - option 3 > replace the hard-coded 16 that can be replaced and specifically > define TASK_COMM_LEN_16 in other files which can't include > linux/sched.h. > > Which one do you prefer ? > Option 3. Since TASK_COMM_LEN_16 is, by it's name, already hard coded to 16, it doesn't really matter if you define it in more than one location. Or we could define it in another header that include/sched.h can include. The idea of having TASK_COMM_LEN_16 is to easily grep for it, and also know exactly what it is used for when people see it being used. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/7] fs/binfmt_elf: replace open-coded string copy with get_task_comm 2021-11-30 14:22 ` Steven Rostedt @ 2021-11-30 15:53 ` Yafang Shao 0 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-30 15:53 UTC (permalink / raw) To: Steven Rostedt Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Kees Cook, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Petr Mladek On Tue, Nov 30, 2021 at 10:22 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 30 Nov 2021 11:01:27 +0800 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > There are three options, > > - option 1 > > comment on all the hard-coded 16 to explain why it is hard-coded > > - option 2 > > replace the hard-coded 16 that can be replaced and comment on the > > others which can't be replaced. > > - option 3 > > replace the hard-coded 16 that can be replaced and specifically > > define TASK_COMM_LEN_16 in other files which can't include > > linux/sched.h. > > > > Which one do you prefer ? > > > > Option 3. Since TASK_COMM_LEN_16 is, by it's name, already hard coded to > 16, it doesn't really matter if you define it in more than one location. > > Or we could define it in another header that include/sched.h can include. > > The idea of having TASK_COMM_LEN_16 is to easily grep for it, and also know > exactly what it is used for when people see it being used. > I will send a separate patch (or patchset) to replace all the old hard-coded 16 with TASK_COMM_LEN_16 based on the -mm tree. -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 5/7] samples/bpf/test_overhead_kprobe_kern: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao ` (3 preceding siblings ...) 2021-11-20 11:27 ` [PATCH v2 4/7] fs/binfmt_elf: " Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 6/7] tools/bpf/bpftool/skeleton: " Yafang Shao 2021-11-20 11:27 ` [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN Yafang Shao 6 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Kees Cook, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Petr Mladek bpf_probe_read_kernel_str() will add a nul terminator to the dst, then we don't care about if the dst size is big enough. This patch also replaces the hard-coded 16 with TASK_COMM_LEN to make it grepable. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org> Acked-by: Andrii Nakryiko <andrii@kernel.org> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- samples/bpf/offwaketime_kern.c | 4 ++-- samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++----- samples/bpf/test_overhead_tp_kern.c | 5 +++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c index 4866afd054da..eb4d94742e6b 100644 --- a/samples/bpf/offwaketime_kern.c +++ b/samples/bpf/offwaketime_kern.c @@ -113,11 +113,11 @@ static inline int update_counts(void *ctx, u32 pid, u64 delta) /* 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]; int prev_pid; int prev_prio; long long prev_state; - char next_comm[16]; + char next_comm[TASK_COMM_LEN]; int next_pid; int next_prio; }; diff --git a/samples/bpf/test_overhead_kprobe_kern.c b/samples/bpf/test_overhead_kprobe_kern.c index f6d593e47037..8fdd2c9c56b2 100644 --- a/samples/bpf/test_overhead_kprobe_kern.c +++ b/samples/bpf/test_overhead_kprobe_kern.c @@ -6,6 +6,7 @@ */ #include <linux/version.h> #include <linux/ptrace.h> +#include <linux/sched.h> #include <uapi/linux/bpf.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> @@ -22,17 +23,17 @@ int prog(struct pt_regs *ctx) { struct signal_struct *signal; struct task_struct *tsk; - char oldcomm[16] = {}; - char newcomm[16] = {}; + char oldcomm[TASK_COMM_LEN] = {}; + char newcomm[TASK_COMM_LEN] = {}; u16 oom_score_adj; u32 pid; tsk = (void *)PT_REGS_PARM1(ctx); pid = _(tsk->pid); - bpf_probe_read_kernel(oldcomm, sizeof(oldcomm), &tsk->comm); - bpf_probe_read_kernel(newcomm, sizeof(newcomm), - (void *)PT_REGS_PARM2(ctx)); + bpf_probe_read_kernel_str(oldcomm, sizeof(oldcomm), &tsk->comm); + bpf_probe_read_kernel_str(newcomm, sizeof(newcomm), + (void *)PT_REGS_PARM2(ctx)); signal = _(tsk->signal); oom_score_adj = _(signal->oom_score_adj); return 0; diff --git a/samples/bpf/test_overhead_tp_kern.c b/samples/bpf/test_overhead_tp_kern.c index eaa32693f8fc..80edadacb692 100644 --- a/samples/bpf/test_overhead_tp_kern.c +++ b/samples/bpf/test_overhead_tp_kern.c @@ -4,6 +4,7 @@ * modify it under the terms of version 2 of the GNU General Public * License as published by the Free Software Foundation. */ +#include <linux/sched.h> #include <uapi/linux/bpf.h> #include <bpf/bpf_helpers.h> @@ -11,8 +12,8 @@ struct task_rename { __u64 pad; __u32 pid; - char oldcomm[16]; - char newcomm[16]; + char oldcomm[TASK_COMM_LEN]; + char newcomm[TASK_COMM_LEN]; __u16 oom_score_adj; }; SEC("tracepoint/task/task_rename") -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 6/7] tools/bpf/bpftool/skeleton: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao ` (4 preceding siblings ...) 2021-11-20 11:27 ` [PATCH v2 5/7] samples/bpf/test_overhead_kprobe_kern: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN Yafang Shao 6 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Alexei Starovoitov, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek bpf_probe_read_kernel_str() will add a nul terminator to the dst, then we don't care about if the dst size is big enough. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Acked-by: Andrii Nakryiko <andrii@kernel.org> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao ` (5 preceding siblings ...) 2021-11-20 11:27 ` [PATCH v2 6/7] tools/bpf/bpftool/skeleton: " Yafang Shao @ 2021-11-20 11:27 ` Yafang Shao 2021-11-29 10:13 ` Sven Schnelle 2023-02-08 21:55 ` John Stultz 6 siblings, 2 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-20 11:27 UTC (permalink / raw) To: akpm Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Yafang Shao, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek As the sched:sched_switch tracepoint args are derived from the kernel, we'd better make it same with the kernel. So the macro TASK_COMM_LEN is converted to type enum, then all the BPF programs can get it through BTF. The BPF program which wants to use TASK_COMM_LEN should include the header vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the type defined in linux/bpf.h are also defined in vmlinux.h, so we don't need to include linux/bpf.h again. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Acked-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: David Hildenbrand <david@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: David Hildenbrand <david@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- include/linux/sched.h | 9 +++++++-- tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index 78c351e35fec..cecd4806edc6 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -274,8 +274,13 @@ struct task_group; #define get_current_state() READ_ONCE(current->__state) -/* Task command name length: */ -#define TASK_COMM_LEN 16 +/* + * Define the task command name length as enum, then it can be visible to + * BPF programs. + */ +enum { + TASK_COMM_LEN = 16, +}; extern void scheduler_tick(void); diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c index a8233e7f173b..728dbd39eff0 100644 --- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c +++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2018 Facebook -#include <linux/bpf.h> +#include <vmlinux.h> #include <bpf/bpf_helpers.h> #ifndef PERF_MAX_STACK_DEPTH @@ -41,11 +41,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]; int prev_pid; int prev_prio; long long prev_state; - char next_comm[16]; + char next_comm[TASK_COMM_LEN]; 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 ce6974016f53..43bd7a20cc50 100644 --- a/tools/testing/selftests/bpf/progs/test_tracepoint.c +++ b/tools/testing/selftests/bpf/progs/test_tracepoint.c @@ -1,17 +1,17 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2017 Facebook -#include <linux/bpf.h> +#include <vmlinux.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]; int prev_pid; int prev_prio; long long prev_state; - char next_comm[16]; + char next_comm[TASK_COMM_LEN]; int next_pid; int next_prio; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-20 11:27 ` [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN Yafang Shao @ 2021-11-29 10:13 ` Sven Schnelle 2021-11-29 13:41 ` Yafang Shao 2021-11-29 17:30 ` Steven Rostedt 2023-02-08 21:55 ` John Stultz 1 sibling, 2 replies; 43+ messages in thread From: Sven Schnelle @ 2021-11-29 10:13 UTC (permalink / raw) To: Yafang Shao Cc: akpm, netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek Hi, Yafang Shao <laoar.shao@gmail.com> writes: > As the sched:sched_switch tracepoint args are derived from the kernel, > we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > converted to type enum, then all the BPF programs can get it through BTF. > > The BPF program which wants to use TASK_COMM_LEN should include the header > vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > need to include linux/bpf.h again. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Acked-by: David Hildenbrand <david@redhat.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: David Hildenbrand <david@redhat.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Kees Cook <keescook@chromium.org> > Cc: Petr Mladek <pmladek@suse.com> > --- > include/linux/sched.h | 9 +++++++-- > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > 3 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 78c351e35fec..cecd4806edc6 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -274,8 +274,13 @@ struct task_group; > > #define get_current_state() READ_ONCE(current->__state) > > -/* Task command name length: */ > -#define TASK_COMM_LEN 16 > +/* > + * Define the task command name length as enum, then it can be visible to > + * BPF programs. > + */ > +enum { > + TASK_COMM_LEN = 16, > +}; This breaks the trigger-field-variable-support.tc from the ftrace test suite at least on s390: echo 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument I added a debugging line into check_synth_field(): [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 Note the difference in the signed field. Regards Sven ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 10:13 ` Sven Schnelle @ 2021-11-29 13:41 ` Yafang Shao 2021-11-29 14:21 ` Sven Schnelle 2021-11-29 17:30 ` Steven Rostedt 1 sibling, 1 reply; 43+ messages in thread From: Yafang Shao @ 2021-11-29 13:41 UTC (permalink / raw) To: Sven Schnelle Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On Mon, Nov 29, 2021 at 6:13 PM Sven Schnelle <svens@linux.ibm.com> wrote: > > Hi, > > Yafang Shao <laoar.shao@gmail.com> writes: > > > As the sched:sched_switch tracepoint args are derived from the kernel, > > we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > > converted to type enum, then all the BPF programs can get it through BTF. > > > > The BPF program which wants to use TASK_COMM_LEN should include the header > > vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > > type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > > need to include linux/bpf.h again. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Acked-by: David Hildenbrand <david@redhat.com> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Matthew Wilcox <willy@infradead.org> > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Petr Mladek <pmladek@suse.com> > > --- > > include/linux/sched.h | 9 +++++++-- > > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > > tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > > 3 files changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 78c351e35fec..cecd4806edc6 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -274,8 +274,13 @@ struct task_group; > > > > #define get_current_state() READ_ONCE(current->__state) > > > > -/* Task command name length: */ > > -#define TASK_COMM_LEN 16 > > +/* > > + * Define the task command name length as enum, then it can be visible to > > + * BPF programs. > > + */ > > +enum { > > + TASK_COMM_LEN = 16, > > +}; > > This breaks the trigger-field-variable-support.tc from the ftrace test > suite at least on s390: > > echo 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' > linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument > > I added a debugging line into check_synth_field(): > > [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 > > Note the difference in the signed field. > Hi Sven, Thanks for the report and debugging! Seems we should explicitly define it as signed ? Could you pls. help verify it? diff --git a/include/linux/sched.h b/include/linux/sched.h index cecd4806edc6..44d36c6af3e1 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -278,7 +278,7 @@ struct task_group; * Define the task command name length as enum, then it can be visible to * BPF programs. */ -enum { +enum SignedEnum { TASK_COMM_LEN = 16, }; -- Thanks Yafang ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 13:41 ` Yafang Shao @ 2021-11-29 14:21 ` Sven Schnelle 2021-11-29 14:32 ` David Hildenbrand 2021-11-29 15:28 ` Yafang Shao 0 siblings, 2 replies; 43+ messages in thread From: Sven Schnelle @ 2021-11-29 14:21 UTC (permalink / raw) To: Yafang Shao Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek Hi, Yafang Shao <laoar.shao@gmail.com> writes: > On Mon, Nov 29, 2021 at 6:13 PM Sven Schnelle <svens@linux.ibm.com> wrote: >> > diff --git a/include/linux/sched.h b/include/linux/sched.h >> > index 78c351e35fec..cecd4806edc6 100644 >> > --- a/include/linux/sched.h >> > +++ b/include/linux/sched.h >> > @@ -274,8 +274,13 @@ struct task_group; >> > >> > #define get_current_state() READ_ONCE(current->__state) >> > >> > -/* Task command name length: */ >> > -#define TASK_COMM_LEN 16 >> > +/* >> > + * Define the task command name length as enum, then it can be visible to >> > + * BPF programs. >> > + */ >> > +enum { >> > + TASK_COMM_LEN = 16, >> > +}; >> >> This breaks the trigger-field-variable-support.tc from the ftrace test >> suite at least on s390: >> >> echo >> 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) >> if next_comm=="ping"' >> linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument >> >> I added a debugging line into check_synth_field(): >> >> [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 >> >> Note the difference in the signed field. >> > > Hi Sven, > > Thanks for the report and debugging! > Seems we should explicitly define it as signed ? > Could you pls. help verify it? > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index cecd4806edc6..44d36c6af3e1 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -278,7 +278,7 @@ struct task_group; > * Define the task command name length as enum, then it can be visible to > * BPF programs. > */ > -enum { > +enum SignedEnum { > TASK_COMM_LEN = 16, > }; Umm no. What you're doing here is to define the name of the enum as 'SignedEnum'. This doesn't change the type. I think before C++0x you couldn't force an enum type. Regards Sven ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 14:21 ` Sven Schnelle @ 2021-11-29 14:32 ` David Hildenbrand 2021-11-29 14:38 ` Sven Schnelle 2021-11-29 15:28 ` Yafang Shao 1 sibling, 1 reply; 43+ messages in thread From: David Hildenbrand @ 2021-11-29 14:32 UTC (permalink / raw) To: Sven Schnelle, Yafang Shao Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On 29.11.21 15:21, Sven Schnelle wrote: > Hi, > > Yafang Shao <laoar.shao@gmail.com> writes: > >> On Mon, Nov 29, 2021 at 6:13 PM Sven Schnelle <svens@linux.ibm.com> wrote: >>>> diff --git a/include/linux/sched.h b/include/linux/sched.h >>>> index 78c351e35fec..cecd4806edc6 100644 >>>> --- a/include/linux/sched.h >>>> +++ b/include/linux/sched.h >>>> @@ -274,8 +274,13 @@ struct task_group; >>>> >>>> #define get_current_state() READ_ONCE(current->__state) >>>> >>>> -/* Task command name length: */ >>>> -#define TASK_COMM_LEN 16 >>>> +/* >>>> + * Define the task command name length as enum, then it can be visible to >>>> + * BPF programs. >>>> + */ >>>> +enum { >>>> + TASK_COMM_LEN = 16, >>>> +}; >>> >>> This breaks the trigger-field-variable-support.tc from the ftrace test >>> suite at least on s390: >>> >>> echo >>> 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) >>> if next_comm=="ping"' >>> linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument >>> >>> I added a debugging line into check_synth_field(): >>> >>> [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 >>> >>> Note the difference in the signed field. >>> >> >> Hi Sven, >> >> Thanks for the report and debugging! >> Seems we should explicitly define it as signed ? >> Could you pls. help verify it? >> >> diff --git a/include/linux/sched.h b/include/linux/sched.h >> index cecd4806edc6..44d36c6af3e1 100644 >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> @@ -278,7 +278,7 @@ struct task_group; >> * Define the task command name length as enum, then it can be visible to >> * BPF programs. >> */ >> -enum { >> +enum SignedEnum { >> TASK_COMM_LEN = 16, >> }; > > Umm no. What you're doing here is to define the name of the enum as > 'SignedEnum'. This doesn't change the type. I think before C++0x you > couldn't force an enum type. I think there are only some "hacks" to modify the type with GCC. For example, with "__attribute__((packed))" we can instruct GCC to use the smallest type possible for the defined enum values. I think with some fake entries one can eventually instruct GCC to use an unsigned type in some cases: https://stackoverflow.com/questions/14635833/is-there-a-way-to-make-an-enum-unsigned-in-the-c90-standard-misra-c-2004-compl enum { TASK_COMM_LEN = 16, TASK_FORCE_UNSIGNED = 0x80000000, }; Haven't tested it, though, and I'm not sure if we should really do that ... :) -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 14:32 ` David Hildenbrand @ 2021-11-29 14:38 ` Sven Schnelle 2021-11-29 15:33 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Sven Schnelle @ 2021-11-29 14:38 UTC (permalink / raw) To: David Hildenbrand Cc: Yafang Shao, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek Hi, David Hildenbrand <david@redhat.com> writes: > On 29.11.21 15:21, Sven Schnelle wrote: >> Yafang Shao <laoar.shao@gmail.com> writes: >>> Thanks for the report and debugging! >>> Seems we should explicitly define it as signed ? >>> Could you pls. help verify it? >>> >>> diff --git a/include/linux/sched.h b/include/linux/sched.h >>> index cecd4806edc6..44d36c6af3e1 100644 >>> --- a/include/linux/sched.h >>> +++ b/include/linux/sched.h >>> @@ -278,7 +278,7 @@ struct task_group; >>> * Define the task command name length as enum, then it can be visible to >>> * BPF programs. >>> */ >>> -enum { >>> +enum SignedEnum { >>> TASK_COMM_LEN = 16, >>> }; >> >> Umm no. What you're doing here is to define the name of the enum as >> 'SignedEnum'. This doesn't change the type. I think before C++0x you >> couldn't force an enum type. > > I think there are only some "hacks" to modify the type with GCC. For > example, with "__attribute__((packed))" we can instruct GCC to use the > smallest type possible for the defined enum values. Yes, i meant no way that the standard defines. You could force it to signed by having a negative member. > I think with some fake entries one can eventually instruct GCC to use an > unsigned type in some cases: > > https://stackoverflow.com/questions/14635833/is-there-a-way-to-make-an-enum-unsigned-in-the-c90-standard-misra-c-2004-compl > > enum { > TASK_COMM_LEN = 16, > TASK_FORCE_UNSIGNED = 0x80000000, > }; > > Haven't tested it, though, and I'm not sure if we should really do that > ... :) TBH, i would vote for reverting the change. defining an array size as enum feels really odd. Regards Sven ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 14:38 ` Sven Schnelle @ 2021-11-29 15:33 ` Yafang Shao 2021-11-29 16:07 ` Steven Rostedt 0 siblings, 1 reply; 43+ messages in thread From: Yafang Shao @ 2021-11-29 15:33 UTC (permalink / raw) To: Sven Schnelle Cc: David Hildenbrand, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On Mon, Nov 29, 2021 at 10:38 PM Sven Schnelle <svens@linux.ibm.com> wrote: > > Hi, > > David Hildenbrand <david@redhat.com> writes: > > On 29.11.21 15:21, Sven Schnelle wrote: > >> Yafang Shao <laoar.shao@gmail.com> writes: > >>> Thanks for the report and debugging! > >>> Seems we should explicitly define it as signed ? > >>> Could you pls. help verify it? > >>> > >>> diff --git a/include/linux/sched.h b/include/linux/sched.h > >>> index cecd4806edc6..44d36c6af3e1 100644 > >>> --- a/include/linux/sched.h > >>> +++ b/include/linux/sched.h > >>> @@ -278,7 +278,7 @@ struct task_group; > >>> * Define the task command name length as enum, then it can be visible to > >>> * BPF programs. > >>> */ > >>> -enum { > >>> +enum SignedEnum { > >>> TASK_COMM_LEN = 16, > >>> }; > >> > >> Umm no. What you're doing here is to define the name of the enum as > >> 'SignedEnum'. This doesn't change the type. I think before C++0x you > >> couldn't force an enum type. > > > > I think there are only some "hacks" to modify the type with GCC. For > > example, with "__attribute__((packed))" we can instruct GCC to use the > > smallest type possible for the defined enum values. > > Yes, i meant no way that the standard defines. You could force it to > signed by having a negative member. > > > I think with some fake entries one can eventually instruct GCC to use an > > unsigned type in some cases: > > > > https://stackoverflow.com/questions/14635833/is-there-a-way-to-make-an-enum-unsigned-in-the-c90-standard-misra-c-2004-compl > > > > enum { > > TASK_COMM_LEN = 16, > > TASK_FORCE_UNSIGNED = 0x80000000, > > }; > > > > Haven't tested it, though, and I'm not sure if we should really do that > > ... :) > > TBH, i would vote for reverting the change. defining an array size as > enum feels really odd. > We changed it to enum because the BTF can't parse macro while it can parse the enum type. Anyway I don't insist on keeping this change if you think reverting it is better. Andrew, would you pls. help drop this patch from the -mm tree (the other 6 patches in this series can be kept) ? -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 15:33 ` Yafang Shao @ 2021-11-29 16:07 ` Steven Rostedt 2021-11-29 16:08 ` Steven Rostedt 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2021-11-29 16:07 UTC (permalink / raw) To: Yafang Shao Cc: Sven Schnelle, David Hildenbrand, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On Mon, 29 Nov 2021 23:33:33 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > TBH, i would vote for reverting the change. defining an array size as > > enum feels really odd. > > > > We changed it to enum because the BTF can't parse macro while it can > parse the enum type. I wonder if BTF could take advantage of the tracing: TRACE_DEFINE_ENUM() macros? This is how they are converted for user space tooling. Anyway, I'd have to go and look at why that trigger test failed. I don't see how the size of the array caused it to change the signage of value. -- Steve > Anyway I don't insist on keeping this change if you think reverting it > is better. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 16:07 ` Steven Rostedt @ 2021-11-29 16:08 ` Steven Rostedt 0 siblings, 0 replies; 43+ messages in thread From: Steven Rostedt @ 2021-11-29 16:08 UTC (permalink / raw) To: Yafang Shao Cc: Sven Schnelle, David Hildenbrand, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On Mon, 29 Nov 2021 11:07:55 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > I wonder if BTF could take advantage of the tracing: > > TRACE_DEFINE_ENUM() macros? Bah' BTF does handle enums, it doesn't handle macros. But I wonder if we could do something similar for BTF. That is, force it. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 14:21 ` Sven Schnelle 2021-11-29 14:32 ` David Hildenbrand @ 2021-11-29 15:28 ` Yafang Shao 1 sibling, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-29 15:28 UTC (permalink / raw) To: Sven Schnelle Cc: Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek On Mon, Nov 29, 2021 at 10:21 PM Sven Schnelle <svens@linux.ibm.com> wrote: > > Hi, > > Yafang Shao <laoar.shao@gmail.com> writes: > > > On Mon, Nov 29, 2021 at 6:13 PM Sven Schnelle <svens@linux.ibm.com> wrote: > >> > diff --git a/include/linux/sched.h b/include/linux/sched.h > >> > index 78c351e35fec..cecd4806edc6 100644 > >> > --- a/include/linux/sched.h > >> > +++ b/include/linux/sched.h > >> > @@ -274,8 +274,13 @@ struct task_group; > >> > > >> > #define get_current_state() READ_ONCE(current->__state) > >> > > >> > -/* Task command name length: */ > >> > -#define TASK_COMM_LEN 16 > >> > +/* > >> > + * Define the task command name length as enum, then it can be visible to > >> > + * BPF programs. > >> > + */ > >> > +enum { > >> > + TASK_COMM_LEN = 16, > >> > +}; > >> > >> This breaks the trigger-field-variable-support.tc from the ftrace test > >> suite at least on s390: > >> > >> echo > >> 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) > >> if next_comm=="ping"' > >> linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument > >> > >> I added a debugging line into check_synth_field(): > >> > >> [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 > >> > >> Note the difference in the signed field. > >> > > > > Hi Sven, > > > > Thanks for the report and debugging! > > Seems we should explicitly define it as signed ? > > Could you pls. help verify it? > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index cecd4806edc6..44d36c6af3e1 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -278,7 +278,7 @@ struct task_group; > > * Define the task command name length as enum, then it can be visible to > > * BPF programs. > > */ > > -enum { > > +enum SignedEnum { > > TASK_COMM_LEN = 16, > > }; > > Umm no. What you're doing here is to define the name of the enum as > 'SignedEnum'. This doesn't change the type. I think before C++0x you > couldn't force an enum type. > Ah, I made a stupid mistake.... -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 10:13 ` Sven Schnelle 2021-11-29 13:41 ` Yafang Shao @ 2021-11-29 17:30 ` Steven Rostedt 2021-11-29 17:56 ` Sven Schnelle 2021-11-30 3:03 ` Yafang Shao 1 sibling, 2 replies; 43+ messages in thread From: Steven Rostedt @ 2021-11-29 17:30 UTC (permalink / raw) To: Sven Schnelle Cc: Yafang Shao, akpm, netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Tom Zanussi On Mon, 29 Nov 2021 11:13:31 +0100 Sven Schnelle <svens@linux.ibm.com> wrote: > This breaks the trigger-field-variable-support.tc from the ftrace test > suite at least on s390: > > echo 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' > linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument > > I added a debugging line into check_synth_field(): > > [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 > > Note the difference in the signed field. That should not break on strings. Does this fix it (if you keep the patch)? -- Steve diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 9555b8e1d1e3..319f9c8ca7e7 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -3757,7 +3757,7 @@ static int check_synth_field(struct synth_event *event, if (strcmp(field->type, hist_field->type) != 0) { if (field->size != hist_field->size || - field->is_signed != hist_field->is_signed) + (!field->is_string && field->is_signed != hist_field->is_signed)) return -EINVAL; } ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 17:30 ` Steven Rostedt @ 2021-11-29 17:56 ` Sven Schnelle 2021-11-30 3:03 ` Yafang Shao 1 sibling, 0 replies; 43+ messages in thread From: Sven Schnelle @ 2021-11-29 17:56 UTC (permalink / raw) To: Steven Rostedt Cc: Yafang Shao, akpm, netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Tom Zanussi Steven Rostedt <rostedt@goodmis.org> writes: > On Mon, 29 Nov 2021 11:13:31 +0100 > Sven Schnelle <svens@linux.ibm.com> wrote: > > >> This breaks the trigger-field-variable-support.tc from the ftrace test >> suite at least on s390: >> >> echo >> 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) >> if next_comm=="ping"' >> linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument >> >> I added a debugging line into check_synth_field(): >> >> [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 >> >> Note the difference in the signed field. > > That should not break on strings. > > Does this fix it (if you keep the patch)? It does. Thanks! > -- Steve > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 9555b8e1d1e3..319f9c8ca7e7 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -3757,7 +3757,7 @@ static int check_synth_field(struct synth_event *event, > > if (strcmp(field->type, hist_field->type) != 0) { > if (field->size != hist_field->size || > - field->is_signed != hist_field->is_signed) > + (!field->is_string && field->is_signed != hist_field->is_signed)) > return -EINVAL; > } > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-29 17:30 ` Steven Rostedt 2021-11-29 17:56 ` Sven Schnelle @ 2021-11-30 3:03 ` Yafang Shao 2021-11-30 14:23 ` Steven Rostedt 1 sibling, 1 reply; 43+ messages in thread From: Yafang Shao @ 2021-11-30 3:03 UTC (permalink / raw) To: Steven Rostedt Cc: Sven Schnelle, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Tom Zanussi On Tue, Nov 30, 2021 at 1:30 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 29 Nov 2021 11:13:31 +0100 > Sven Schnelle <svens@linux.ibm.com> wrote: > > > > This breaks the trigger-field-variable-support.tc from the ftrace test > > suite at least on s390: > > > > echo 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' > > linux/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-field-variable-support.tc: line 15: echo: write error: Invalid argument > > > > I added a debugging line into check_synth_field(): > > > > [ 44.091037] field->size 16, hist_field->size 16, field->is_signed 1, hist_field->is_signed 0 > > > > Note the difference in the signed field. > > That should not break on strings. > > Does this fix it (if you keep the patch)? > > -- Steve > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 9555b8e1d1e3..319f9c8ca7e7 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -3757,7 +3757,7 @@ static int check_synth_field(struct synth_event *event, > > if (strcmp(field->type, hist_field->type) != 0) { > if (field->size != hist_field->size || > - field->is_signed != hist_field->is_signed) > + (!field->is_string && field->is_signed != hist_field->is_signed)) > return -EINVAL; > } > Many thanks for the quick fix! It seems this fix should be ahead of patch #7. I will send v3 which contains your fix. -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-30 3:03 ` Yafang Shao @ 2021-11-30 14:23 ` Steven Rostedt 2021-11-30 15:46 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2021-11-30 14:23 UTC (permalink / raw) To: Yafang Shao Cc: Sven Schnelle, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Tom Zanussi On Tue, 30 Nov 2021 11:03:48 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > Many thanks for the quick fix! > It seems this fix should be ahead of patch #7. > I will send v3 which contains your fix. Don't bother. I'm actually going to send this to Linus as a bug fix. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-30 14:23 ` Steven Rostedt @ 2021-11-30 15:46 ` Yafang Shao 0 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2021-11-30 15:46 UTC (permalink / raw) To: Steven Rostedt Cc: Sven Schnelle, Andrew Morton, netdev, bpf, linux-perf-use., linux-fsdevel, Linux MM, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Tom Zanussi On Tue, Nov 30, 2021 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 30 Nov 2021 11:03:48 +0800 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > Many thanks for the quick fix! > > It seems this fix should be ahead of patch #7. > > I will send v3 which contains your fix. > > Don't bother. I'm actually going to send this to Linus as a bug fix. > Great! Thanks for the work. -- Thanks Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2021-11-20 11:27 ` [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN Yafang Shao 2021-11-29 10:13 ` Sven Schnelle @ 2023-02-08 21:55 ` John Stultz 2023-02-09 0:10 ` Alexei Starovoitov 1 sibling, 1 reply; 43+ messages in thread From: John Stultz @ 2023-02-08 21:55 UTC (permalink / raw) To: Yafang Shao Cc: akpm, netdev, bpf, linux-perf-users, linux-fsdevel, linux-mm, linux-kernel, oliver.sang, lkp, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Sat, Nov 20, 2021 at 11:27:38AM +0000, Yafang Shao wrote: > As the sched:sched_switch tracepoint args are derived from the kernel, > we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > converted to type enum, then all the BPF programs can get it through BTF. > > The BPF program which wants to use TASK_COMM_LEN should include the header > vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > need to include linux/bpf.h again. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Acked-by: David Hildenbrand <david@redhat.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: David Hildenbrand <david@redhat.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Kees Cook <keescook@chromium.org> > Cc: Petr Mladek <pmladek@suse.com> > --- > include/linux/sched.h | 9 +++++++-- > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > 3 files changed, 13 insertions(+), 8 deletions(-) Hey all, I know this is a little late, but I recently got a report that this change was causiing older versions of perfetto to stop working. Apparently newer versions of perfetto has worked around this via the following changes: https://android.googlesource.com/platform/external/perfetto/+/c717c93131b1b6e3705a11092a70ac47c78b731d%5E%21/ https://android.googlesource.com/platform/external/perfetto/+/160a504ad5c91a227e55f84d3e5d3fe22af7c2bb%5E%21/ But for older versions of perfetto, reverting upstream commit 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN") is necessary to get it back to working. I haven't dug very far into the details, and obviously this doesn't break with the updated perfetto, but from a high level this does seem to be a breaking-userland regression. So I wanted to reach out to see if there was more context for this breakage? I don't want to raise a unnecessary stink if this was an unfortuante but forced situation. thanks -john ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-08 21:55 ` John Stultz @ 2023-02-09 0:10 ` Alexei Starovoitov 2023-02-09 0:54 ` John Stultz 0 siblings, 1 reply; 43+ messages in thread From: Alexei Starovoitov @ 2023-02-09 0:10 UTC (permalink / raw) To: John Stultz Cc: Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Wed, Feb 8, 2023 at 2:01 PM John Stultz <jstultz@google.com> wrote: > > On Sat, Nov 20, 2021 at 11:27:38AM +0000, Yafang Shao wrote: > > As the sched:sched_switch tracepoint args are derived from the kernel, > > we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > > converted to type enum, then all the BPF programs can get it through BTF. > > > > The BPF program which wants to use TASK_COMM_LEN should include the header > > vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > > type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > > need to include linux/bpf.h again. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Acked-by: David Hildenbrand <david@redhat.com> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Matthew Wilcox <willy@infradead.org> > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Petr Mladek <pmladek@suse.com> > > --- > > include/linux/sched.h | 9 +++++++-- > > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > > tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > > 3 files changed, 13 insertions(+), 8 deletions(-) > > Hey all, > I know this is a little late, but I recently got a report that > this change was causiing older versions of perfetto to stop > working. > > Apparently newer versions of perfetto has worked around this > via the following changes: > https://android.googlesource.com/platform/external/perfetto/+/c717c93131b1b6e3705a11092a70ac47c78b731d%5E%21/ > https://android.googlesource.com/platform/external/perfetto/+/160a504ad5c91a227e55f84d3e5d3fe22af7c2bb%5E%21/ > > But for older versions of perfetto, reverting upstream commit > 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 > with TASK_COMM_LEN") is necessary to get it back to working. > > I haven't dug very far into the details, and obviously this doesn't > break with the updated perfetto, but from a high level this does > seem to be a breaking-userland regression. > > So I wanted to reach out to see if there was more context for this > breakage? I don't want to raise a unnecessary stink if this was > an unfortuante but forced situation. Let me understand what you're saying... The commit 3087c61ed2c4 did -/* Task command name length: */ -#define TASK_COMM_LEN 16 +/* + * Define the task command name length as enum, then it can be visible to + * BPF programs. + */ +enum { + TASK_COMM_LEN = 16, +}; and that caused: cat /sys/kernel/debug/tracing/events/task/task_newtask/format to print field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; instead of field:char comm[16]; offset:12; size:16; signed:0; so the ftrace parsing android tracing tool had to do: - if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) { + if (Match(type_and_name.c_str(), + R"(char [a-zA-Z_][a-zA-Z_0-9]*\[[a-zA-Z_0-9]+\])")) { to workaround this change. Right? And what are you proposing? ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 0:10 ` Alexei Starovoitov @ 2023-02-09 0:54 ` John Stultz 2023-02-09 2:06 ` Mathieu Desnoyers 2023-02-09 2:28 ` Steven Rostedt 0 siblings, 2 replies; 43+ messages in thread From: John Stultz @ 2023-02-09 0:54 UTC (permalink / raw) To: Alexei Starovoitov Cc: Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Wed, Feb 8, 2023 at 4:11 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Feb 8, 2023 at 2:01 PM John Stultz <jstultz@google.com> wrote: > > > > On Sat, Nov 20, 2021 at 11:27:38AM +0000, Yafang Shao wrote: > > > As the sched:sched_switch tracepoint args are derived from the kernel, > > > we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > > > converted to type enum, then all the BPF programs can get it through BTF. > > > > > > The BPF program which wants to use TASK_COMM_LEN should include the header > > > vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > > > type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > > > need to include linux/bpf.h again. > > > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > Acked-by: David Hildenbrand <david@redhat.com> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > Cc: Matthew Wilcox <willy@infradead.org> > > > Cc: David Hildenbrand <david@redhat.com> > > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Petr Mladek <pmladek@suse.com> > > > --- > > > include/linux/sched.h | 9 +++++++-- > > > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > > > tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > > > 3 files changed, 13 insertions(+), 8 deletions(-) > > > > Hey all, > > I know this is a little late, but I recently got a report that > > this change was causiing older versions of perfetto to stop > > working. > > > > Apparently newer versions of perfetto has worked around this > > via the following changes: > > https://android.googlesource.com/platform/external/perfetto/+/c717c93131b1b6e3705a11092a70ac47c78b731d%5E%21/ > > https://android.googlesource.com/platform/external/perfetto/+/160a504ad5c91a227e55f84d3e5d3fe22af7c2bb%5E%21/ > > > > But for older versions of perfetto, reverting upstream commit > > 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 > > with TASK_COMM_LEN") is necessary to get it back to working. > > > > I haven't dug very far into the details, and obviously this doesn't > > break with the updated perfetto, but from a high level this does > > seem to be a breaking-userland regression. > > > > So I wanted to reach out to see if there was more context for this > > breakage? I don't want to raise a unnecessary stink if this was > > an unfortuante but forced situation. > > Let me understand what you're saying... > > The commit 3087c61ed2c4 did > > -/* Task command name length: */ > -#define TASK_COMM_LEN 16 > +/* > + * Define the task command name length as enum, then it can be visible to > + * BPF programs. > + */ > +enum { > + TASK_COMM_LEN = 16, > +}; > > > and that caused: > > cat /sys/kernel/debug/tracing/events/task/task_newtask/format > > to print > field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; > instead of > field:char comm[16]; offset:12; size:16; signed:0; > > so the ftrace parsing android tracing tool had to do: > > - if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) { > + if (Match(type_and_name.c_str(), > + R"(char [a-zA-Z_][a-zA-Z_0-9]*\[[a-zA-Z_0-9]+\])")) { > > to workaround this change. > Right? I believe so. > And what are you proposing? I'm not proposing anything. I was just wanting to understand more context around this, as it outwardly appears to be a user-breaking change, and that is usually not done, so I figured it was an issue worth raising. If the debug/tracing/*/format output is in the murky not-really-abi space, that's fine, but I wanted to know if this was understood as something that may require userland updates or if this was a unexpected side-effect. thanks -john ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 0:54 ` John Stultz @ 2023-02-09 2:06 ` Mathieu Desnoyers 2023-02-09 6:20 ` Yafang Shao 2023-02-09 2:28 ` Steven Rostedt 1 sibling, 1 reply; 43+ messages in thread From: Mathieu Desnoyers @ 2023-02-09 2:06 UTC (permalink / raw) To: John Stultz, Alexei Starovoitov Cc: Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto On 2023-02-08 19:54, John Stultz wrote: > On Wed, Feb 8, 2023 at 4:11 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Wed, Feb 8, 2023 at 2:01 PM John Stultz <jstultz@google.com> wrote: >>> >>> On Sat, Nov 20, 2021 at 11:27:38AM +0000, Yafang Shao wrote: >>>> As the sched:sched_switch tracepoint args are derived from the kernel, >>>> we'd better make it same with the kernel. So the macro TASK_COMM_LEN is >>>> converted to type enum, then all the BPF programs can get it through BTF. >>>> >>>> The BPF program which wants to use TASK_COMM_LEN should include the header >>>> vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the >>>> type defined in linux/bpf.h are also defined in vmlinux.h, so we don't >>>> need to include linux/bpf.h again. >>>> >>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >>>> Acked-by: Andrii Nakryiko <andrii@kernel.org> >>>> Acked-by: David Hildenbrand <david@redhat.com> >>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >>>> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> >>>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> >>>> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> >>>> Cc: Peter Zijlstra <peterz@infradead.org> >>>> Cc: Steven Rostedt <rostedt@goodmis.org> >>>> Cc: Matthew Wilcox <willy@infradead.org> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Cc: Al Viro <viro@zeniv.linux.org.uk> >>>> Cc: Kees Cook <keescook@chromium.org> >>>> Cc: Petr Mladek <pmladek@suse.com> >>>> --- >>>> include/linux/sched.h | 9 +++++++-- >>>> tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- >>>> tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- >>>> 3 files changed, 13 insertions(+), 8 deletions(-) >>> >>> Hey all, >>> I know this is a little late, but I recently got a report that >>> this change was causiing older versions of perfetto to stop >>> working. >>> >>> Apparently newer versions of perfetto has worked around this >>> via the following changes: >>> https://android.googlesource.com/platform/external/perfetto/+/c717c93131b1b6e3705a11092a70ac47c78b731d%5E%21/ >>> https://android.googlesource.com/platform/external/perfetto/+/160a504ad5c91a227e55f84d3e5d3fe22af7c2bb%5E%21/ >>> >>> But for older versions of perfetto, reverting upstream commit >>> 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 >>> with TASK_COMM_LEN") is necessary to get it back to working. >>> >>> I haven't dug very far into the details, and obviously this doesn't >>> break with the updated perfetto, but from a high level this does >>> seem to be a breaking-userland regression. >>> >>> So I wanted to reach out to see if there was more context for this >>> breakage? I don't want to raise a unnecessary stink if this was >>> an unfortuante but forced situation. >> >> Let me understand what you're saying... >> >> The commit 3087c61ed2c4 did >> >> -/* Task command name length: */ >> -#define TASK_COMM_LEN 16 >> +/* >> + * Define the task command name length as enum, then it can be visible to >> + * BPF programs. >> + */ >> +enum { >> + TASK_COMM_LEN = 16, >> +}; >> >> >> and that caused: >> >> cat /sys/kernel/debug/tracing/events/task/task_newtask/format >> >> to print >> field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; >> instead of >> field:char comm[16]; offset:12; size:16; signed:0; >> >> so the ftrace parsing android tracing tool had to do: >> >> - if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) { >> + if (Match(type_and_name.c_str(), >> + R"(char [a-zA-Z_][a-zA-Z_0-9]*\[[a-zA-Z_0-9]+\])")) { >> >> to workaround this change. >> Right? > > I believe so. > >> And what are you proposing? > > I'm not proposing anything. I was just wanting to understand more > context around this, as it outwardly appears to be a user-breaking > change, and that is usually not done, so I figured it was an issue > worth raising. > > If the debug/tracing/*/format output is in the murky not-really-abi > space, that's fine, but I wanted to know if this was understood as > something that may require userland updates or if this was a > unexpected side-effect. If you are looking at the root cause in the kernel code generating this: kernel/trace/trace_events.c:f_show() /* * Smartly shows the array type(except dynamic array). * Normal: * field:TYPE VAR * If TYPE := TYPE[LEN], it is shown: * field:TYPE VAR[LEN] */ where it uses the content of field->type (a string) to format the VAR[LEN] part. This in turn is the result of the definition of the struct trace_event_fields done in: include/trace/trace_events.h at stage 4, thus with the context of those macros defined: include/trace/stages/stage4_event_fields.h: #undef __array #define __array(_type, _item, _len) { \ .type = #_type"["__stringify(_len)"]", .name = #_item, \ .size = sizeof(_type[_len]), .align = ALIGN_STRUCTFIELD(_type), \ .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER }, I suspect the real culprit here is the use of __stringify(_len), which happens to work on macros, but not on enum labels. One possible solution to make this more robust would be to extend struct trace_event_fields with one more field that indicates the length of an array as an actual integer, without storing it in its stringified form in the type, and do the formatting in f_show where it belongs. This way everybody can stay happy and no ABI is broken. Thoughts ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 2:06 ` Mathieu Desnoyers @ 2023-02-09 6:20 ` Yafang Shao 2023-02-09 14:27 ` Kajetan Puchalski 0 siblings, 1 reply; 43+ messages in thread From: Yafang Shao @ 2023-02-09 6:20 UTC (permalink / raw) To: Mathieu Desnoyers Cc: John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto [-- Attachment #1: Type: text/plain, Size: 6197 bytes --] On Thu, Feb 9, 2023 at 10:07 AM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > On 2023-02-08 19:54, John Stultz wrote: > > On Wed, Feb 8, 2023 at 4:11 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Wed, Feb 8, 2023 at 2:01 PM John Stultz <jstultz@google.com> wrote: > >>> > >>> On Sat, Nov 20, 2021 at 11:27:38AM +0000, Yafang Shao wrote: > >>>> As the sched:sched_switch tracepoint args are derived from the kernel, > >>>> we'd better make it same with the kernel. So the macro TASK_COMM_LEN is > >>>> converted to type enum, then all the BPF programs can get it through BTF. > >>>> > >>>> The BPF program which wants to use TASK_COMM_LEN should include the header > >>>> vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the > >>>> type defined in linux/bpf.h are also defined in vmlinux.h, so we don't > >>>> need to include linux/bpf.h again. > >>>> > >>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > >>>> Acked-by: Andrii Nakryiko <andrii@kernel.org> > >>>> Acked-by: David Hildenbrand <david@redhat.com> > >>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > >>>> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> > >>>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > >>>> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl> > >>>> Cc: Peter Zijlstra <peterz@infradead.org> > >>>> Cc: Steven Rostedt <rostedt@goodmis.org> > >>>> Cc: Matthew Wilcox <willy@infradead.org> > >>>> Cc: David Hildenbrand <david@redhat.com> > >>>> Cc: Al Viro <viro@zeniv.linux.org.uk> > >>>> Cc: Kees Cook <keescook@chromium.org> > >>>> Cc: Petr Mladek <pmladek@suse.com> > >>>> --- > >>>> include/linux/sched.h | 9 +++++++-- > >>>> tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++--- > >>>> tools/testing/selftests/bpf/progs/test_tracepoint.c | 6 +++--- > >>>> 3 files changed, 13 insertions(+), 8 deletions(-) > >>> > >>> Hey all, > >>> I know this is a little late, but I recently got a report that > >>> this change was causiing older versions of perfetto to stop > >>> working. > >>> > >>> Apparently newer versions of perfetto has worked around this > >>> via the following changes: > >>> https://android.googlesource.com/platform/external/perfetto/+/c717c93131b1b6e3705a11092a70ac47c78b731d%5E%21/ > >>> https://android.googlesource.com/platform/external/perfetto/+/160a504ad5c91a227e55f84d3e5d3fe22af7c2bb%5E%21/ > >>> > >>> But for older versions of perfetto, reverting upstream commit > >>> 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 > >>> with TASK_COMM_LEN") is necessary to get it back to working. > >>> > >>> I haven't dug very far into the details, and obviously this doesn't > >>> break with the updated perfetto, but from a high level this does > >>> seem to be a breaking-userland regression. > >>> > >>> So I wanted to reach out to see if there was more context for this > >>> breakage? I don't want to raise a unnecessary stink if this was > >>> an unfortuante but forced situation. > >> > >> Let me understand what you're saying... > >> > >> The commit 3087c61ed2c4 did > >> > >> -/* Task command name length: */ > >> -#define TASK_COMM_LEN 16 > >> +/* > >> + * Define the task command name length as enum, then it can be visible to > >> + * BPF programs. > >> + */ > >> +enum { > >> + TASK_COMM_LEN = 16, > >> +}; > >> > >> > >> and that caused: > >> > >> cat /sys/kernel/debug/tracing/events/task/task_newtask/format > >> > >> to print > >> field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; > >> instead of > >> field:char comm[16]; offset:12; size:16; signed:0; > >> > >> so the ftrace parsing android tracing tool had to do: > >> > >> - if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) { > >> + if (Match(type_and_name.c_str(), > >> + R"(char [a-zA-Z_][a-zA-Z_0-9]*\[[a-zA-Z_0-9]+\])")) { > >> > >> to workaround this change. > >> Right? > > > > I believe so. > > > >> And what are you proposing? > > > > I'm not proposing anything. I was just wanting to understand more > > context around this, as it outwardly appears to be a user-breaking > > change, and that is usually not done, so I figured it was an issue > > worth raising. > > > > If the debug/tracing/*/format output is in the murky not-really-abi > > space, that's fine, but I wanted to know if this was understood as > > something that may require userland updates or if this was a > > unexpected side-effect. > > If you are looking at the root cause in the kernel code generating this: > > kernel/trace/trace_events.c:f_show() > > /* > * Smartly shows the array type(except dynamic array). > * Normal: > * field:TYPE VAR > * If TYPE := TYPE[LEN], it is shown: > * field:TYPE VAR[LEN] > */ > > where it uses the content of field->type (a string) to format the VAR[LEN] part. > > This in turn is the result of the definition of the > struct trace_event_fields done in: > > include/trace/trace_events.h at stage 4, thus with the context of those macros defined: > > include/trace/stages/stage4_event_fields.h: > > #undef __array > #define __array(_type, _item, _len) { \ > .type = #_type"["__stringify(_len)"]", .name = #_item, \ > .size = sizeof(_type[_len]), .align = ALIGN_STRUCTFIELD(_type), \ > .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER }, > > I suspect the real culprit here is the use of __stringify(_len), which happens to work > on macros, but not on enum labels. > > One possible solution to make this more robust would be to extend > struct trace_event_fields with one more field that indicates the length > of an array as an actual integer, without storing it in its stringified > form in the type, and do the formatting in f_show where it belongs. > > This way everybody can stay happy and no ABI is broken. > > Thoughts ? Many thanks for the detailed analysis. Seems it can work. Hi John, Could you pls. try the attached fix ? I have verified it in my test env. -- Regards Yafang [-- Attachment #2: 0001-trace-fix-TASK_COMM_LEN-in-trace-event-format-file.patch --] [-- Type: application/octet-stream, Size: 5019 bytes --] From 4e6227e65716001aa7861168c3c510317a3774d4 Mon Sep 17 00:00:00 2001 From: Yafang Shao <laoar.shao@gmail.com> Date: Thu, 9 Feb 2023 14:13:48 +0800 Subject: [PATCH 1/1] trace: fix TASK_COMM_LEN in trace event format file Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/trace_events.h | 1 + include/trace/stages/stage4_event_fields.h | 3 ++- kernel/trace/trace.h | 1 + kernel/trace/trace_events.c | 32 ++++++++++++++++++++++-------- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index 4342e99..0e37322 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -270,6 +270,7 @@ struct trace_event_fields { const int align; const int is_signed; const int filter_type; + const int len; }; int (*define_fields)(struct trace_event_call *); }; diff --git a/include/trace/stages/stage4_event_fields.h b/include/trace/stages/stage4_event_fields.h index affd541..306f39a 100644 --- a/include/trace/stages/stage4_event_fields.h +++ b/include/trace/stages/stage4_event_fields.h @@ -26,7 +26,8 @@ #define __array(_type, _item, _len) { \ .type = #_type"["__stringify(_len)"]", .name = #_item, \ .size = sizeof(_type[_len]), .align = ALIGN_STRUCTFIELD(_type), \ - .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER }, + .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER, \ + .len = _len}, #undef __dynamic_array #define __dynamic_array(_type, _item, _len) { \ diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index e46a492..19caf15 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -1282,6 +1282,7 @@ struct ftrace_event_field { int offset; int size; int is_signed; + int len; }; struct prog_entry; diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index 33e0b4f..8419e48 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -114,7 +114,7 @@ struct ftrace_event_field * static int __trace_define_field(struct list_head *head, const char *type, const char *name, int offset, int size, - int is_signed, int filter_type) + int is_signed, int filter_type, int len) { struct ftrace_event_field *field; @@ -133,6 +133,7 @@ static int __trace_define_field(struct list_head *head, const char *type, field->offset = offset; field->size = size; field->is_signed = is_signed; + field->len = len; list_add(&field->link, head); @@ -150,14 +151,28 @@ int trace_define_field(struct trace_event_call *call, const char *type, head = trace_get_fields(call); return __trace_define_field(head, type, name, offset, size, - is_signed, filter_type); + is_signed, filter_type, 0); } EXPORT_SYMBOL_GPL(trace_define_field); +int trace_define_field_ext(struct trace_event_call *call, const char *type, + const char *name, int offset, int size, int is_signed, + int filter_type, int len) +{ + struct list_head *head; + + if (WARN_ON(!call->class)) + return 0; + + head = trace_get_fields(call); + return __trace_define_field(head, type, name, offset, size, + is_signed, filter_type, len); +} + #define __generic_field(type, item, filter_type) \ ret = __trace_define_field(&ftrace_generic_fields, #type, \ #item, 0, 0, is_signed_type(type), \ - filter_type); \ + filter_type, 0); \ if (ret) \ return ret; @@ -166,7 +181,7 @@ int trace_define_field(struct trace_event_call *call, const char *type, "common_" #item, \ offsetof(typeof(ent), item), \ sizeof(ent.item), \ - is_signed_type(type), FILTER_OTHER); \ + is_signed_type(type), FILTER_OTHER, 0); \ if (ret) \ return ret; @@ -1589,10 +1604,10 @@ static int f_show(struct seq_file *m, void *v) field->type, field->name, field->offset, field->size, !!field->is_signed); else - seq_printf(m, "\tfield:%.*s %s%s;\toffset:%u;\tsize:%u;\tsigned:%d;\n", + seq_printf(m, "\tfield:%.*s %s[%d];\toffset:%u;\tsize:%u;\tsigned:%d;\n", (int)(array_descriptor - field->type), field->type, field->name, - array_descriptor, field->offset, + field->len, field->offset, field->size, !!field->is_signed); return 0; @@ -2379,9 +2394,10 @@ static int ftrace_event_release(struct inode *inode, struct file *file) } offset = ALIGN(offset, field->align); - ret = trace_define_field(call, field->type, field->name, + ret = trace_define_field_ext(call, field->type, field->name, offset, field->size, - field->is_signed, field->filter_type); + field->is_signed, field->filter_type, + field->len); if (WARN_ON_ONCE(ret)) { pr_err("error code is %d\n", ret); break; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 6:20 ` Yafang Shao @ 2023-02-09 14:27 ` Kajetan Puchalski 2023-02-09 15:37 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Kajetan Puchalski @ 2023-02-09 14:27 UTC (permalink / raw) To: Yafang Shao Cc: Mathieu Desnoyers, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Thu, Feb 09, 2023 at 02:20:36PM +0800, Yafang Shao wrote: [...] Hi Yafang, > Many thanks for the detailed analysis. Seems it can work. > > Hi John, > > Could you pls. try the attached fix ? I have verified it in my test env. I tested the patch on my environment where I found the issue with newer kernels + older Perfetto. The patch does improve things so that's nice. It goes from "not working at all" to "mostly working but missing data" compared to what happens if I just revert 3087c61ed2c48548b74dd343a5209b87082c682d. I'm just an end user so can't really speak to the underlying causes but for those more familiar with how Perfetto works this is what I'm getting: Error stats for this trace: name idx source value ---------------------------------------- ---------------------------------------- ---------------------------------------- ---------------------------------------- mismatched_sched_switch_tids [NULL] analysis 11101 systrace_parse_failure [NULL] analysis 19040 The trace explorer window ends up containing the ftrace-specific tracks but missing the tracks related to Android-specific callbacks and such. Debug stats below in case they're relevant: Name Value Type android_br_parse_errors 0 error (trace) android_log_format_invalid 0 error (trace) android_log_num_failed 0 error (trace) android_log_num_skipped 0 info (trace) android_log_num_total 0 info (trace) clock_sync_cache_miss 181 info (analysis) clock_sync_failure 0 error (analysis) compact_sched_has_parse_errors 0 error (trace) compact_sched_switch_skipped 0 info (analysis) compact_sched_waking_skipped 0 info (analysis) counter_events_out_of_order 0 error (analysis) deobfuscate_location_parse_error 0 error (trace) empty_chrome_metadata 0 error (trace) energy_breakdown_missing_values 0 error (analysis) energy_descriptor_invalid 0 error (analysis) energy_uid_breakdown_missing_values 0 error (analysis) flow_duplicate_id 0 error (trace) flow_end_without_start 0 info (trace) flow_invalid_id 0 error (trace) flow_no_enclosing_slice 0 error (trace) flow_step_without_start 0 info (trace) flow_without_direction 0 error (trace) frame_timeline_event_parser_errors 0 info (analysis) ftrace_bundle_tokenizer_errors 0 error (analysis) ftrace_cpu_bytes_read_begin[0] 0 info (trace) ftrace_cpu_bytes_read_begin[1] 264 info (trace) ftrace_cpu_bytes_read_begin[2] 0 info (trace) ftrace_cpu_bytes_read_begin[3] 224 info (trace) ftrace_cpu_bytes_read_begin[4] 0 info (trace) ftrace_cpu_bytes_read_begin[5] 0 info (trace) ftrace_cpu_bytes_read_begin[6] 0 info (trace) ftrace_cpu_bytes_read_begin[7] 0 info (trace) ftrace_cpu_bytes_read_delta[0] 6919836 info (trace) ftrace_cpu_bytes_read_delta[1] 7197556 info (trace) ftrace_cpu_bytes_read_delta[2] 6381828 info (trace) ftrace_cpu_bytes_read_delta[3] 5988336 info (trace) ftrace_cpu_bytes_read_delta[4] 5933528 info (trace) ftrace_cpu_bytes_read_delta[5] 4858400 info (trace) ftrace_cpu_bytes_read_delta[6] 6175260 info (trace) ftrace_cpu_bytes_read_delta[7] 4633460 info (trace) ftrace_cpu_bytes_read_end[0] 6919836 info (trace) ftrace_cpu_bytes_read_end[1] 7197820 info (trace) ftrace_cpu_bytes_read_end[2] 6381828 info (trace) ftrace_cpu_bytes_read_end[3] 5988560 info (trace) ftrace_cpu_bytes_read_end[4] 5933528 info (trace) ftrace_cpu_bytes_read_end[5] 4858400 info (trace) ftrace_cpu_bytes_read_end[6] 6175260 info (trace) ftrace_cpu_bytes_read_end[7] 4633460 info (trace) ftrace_cpu_commit_overrun_begin[0] 0 info (trace) ftrace_cpu_commit_overrun_begin[1] 0 info (trace) ftrace_cpu_commit_overrun_begin[2] 0 info (trace) ftrace_cpu_commit_overrun_begin[3] 0 info (trace) ftrace_cpu_commit_overrun_begin[4] 0 info (trace) ftrace_cpu_commit_overrun_begin[5] 0 info (trace) ftrace_cpu_commit_overrun_begin[6] 0 info (trace) ftrace_cpu_commit_overrun_begin[7] 0 info (trace) ftrace_cpu_commit_overrun_delta[0] 0 error (trace) ftrace_cpu_commit_overrun_delta[1] 0 error (trace) ftrace_cpu_commit_overrun_delta[2] 0 error (trace) ftrace_cpu_commit_overrun_delta[3] 0 error (trace) ftrace_cpu_commit_overrun_delta[4] 0 error (trace) ftrace_cpu_commit_overrun_delta[5] 0 error (trace) ftrace_cpu_commit_overrun_delta[6] 0 error (trace) ftrace_cpu_commit_overrun_delta[7] 0 error (trace) ftrace_cpu_commit_overrun_end[0] 0 info (trace) ftrace_cpu_commit_overrun_end[1] 0 info (trace) ftrace_cpu_commit_overrun_end[2] 0 info (trace) ftrace_cpu_commit_overrun_end[3] 0 info (trace) ftrace_cpu_commit_overrun_end[4] 0 info (trace) ftrace_cpu_commit_overrun_end[5] 0 info (trace) ftrace_cpu_commit_overrun_end[6] 0 info (trace) ftrace_cpu_commit_overrun_end[7] 0 info (trace) ftrace_cpu_dropped_events_begin[0] 0 info (trace) ftrace_cpu_dropped_events_begin[1] 0 info (trace) ftrace_cpu_dropped_events_begin[2] 0 info (trace) ftrace_cpu_dropped_events_begin[3] 0 info (trace) ftrace_cpu_dropped_events_begin[4] 0 info (trace) ftrace_cpu_dropped_events_begin[5] 0 info (trace) ftrace_cpu_dropped_events_begin[6] 0 info (trace) ftrace_cpu_dropped_events_begin[7] 0 info (trace) ftrace_cpu_dropped_events_delta[0] 0 error (trace) ftrace_cpu_dropped_events_delta[1] 0 error (trace) ftrace_cpu_dropped_events_delta[2] 0 error (trace) ftrace_cpu_dropped_events_delta[3] 0 error (trace) ftrace_cpu_dropped_events_delta[4] 0 error (trace) ftrace_cpu_dropped_events_delta[5] 0 error (trace) ftrace_cpu_dropped_events_delta[6] 0 error (trace) ftrace_cpu_dropped_events_delta[7] 0 error (trace) ftrace_cpu_dropped_events_end[0] 0 info (trace) ftrace_cpu_dropped_events_end[1] 0 info (trace) ftrace_cpu_dropped_events_end[2] 0 info (trace) ftrace_cpu_dropped_events_end[3] 0 info (trace) ftrace_cpu_dropped_events_end[4] 0 info (trace) ftrace_cpu_dropped_events_end[5] 0 info (trace) ftrace_cpu_dropped_events_end[6] 0 info (trace) ftrace_cpu_dropped_events_end[7] 0 info (trace) ftrace_cpu_entries_begin[0] 0 info (trace) ftrace_cpu_entries_begin[1] 6 info (trace) ftrace_cpu_entries_begin[2] 0 info (trace) ftrace_cpu_entries_begin[3] 5 info (trace) ftrace_cpu_entries_begin[4] 0 info (trace) ftrace_cpu_entries_begin[5] 0 info (trace) ftrace_cpu_entries_begin[6] 0 info (trace) ftrace_cpu_entries_begin[7] 0 info (trace) ftrace_cpu_entries_delta[0] 6 info (trace) ftrace_cpu_entries_delta[1] -6 info (trace) ftrace_cpu_entries_delta[2] 0 info (trace) ftrace_cpu_entries_delta[3] 2 info (trace) ftrace_cpu_entries_delta[4] 0 info (trace) ftrace_cpu_entries_delta[5] 0 info (trace) ftrace_cpu_entries_delta[6] 0 info (trace) ftrace_cpu_entries_delta[7] 0 info (trace) ftrace_cpu_entries_end[0] 6 info (trace) ftrace_cpu_entries_end[1] 0 info (trace) ftrace_cpu_entries_end[2] 0 info (trace) ftrace_cpu_entries_end[3] 7 info (trace) ftrace_cpu_entries_end[4] 0 info (trace) ftrace_cpu_entries_end[5] 0 info (trace) ftrace_cpu_entries_end[6] 0 info (trace) ftrace_cpu_entries_end[7] 0 info (trace) ftrace_cpu_now_ts_begin[0] 93305027000 info (trace) ftrace_cpu_now_ts_begin[1] 93305103000 info (trace) ftrace_cpu_now_ts_begin[2] 93305159000 info (trace) ftrace_cpu_now_ts_begin[3] 93305207000 info (trace) ftrace_cpu_now_ts_begin[4] 93305262000 info (trace) ftrace_cpu_now_ts_begin[5] 93305312000 info (trace) ftrace_cpu_now_ts_begin[6] 93305362000 info (trace) ftrace_cpu_now_ts_begin[7] 93305411000 info (trace) ftrace_cpu_now_ts_end[0] 282906571000 info (trace) ftrace_cpu_now_ts_end[1] 282906676000 info (trace) ftrace_cpu_now_ts_end[2] 282906738000 info (trace) ftrace_cpu_now_ts_end[3] 282906803000 info (trace) ftrace_cpu_now_ts_end[4] 282906863000 info (trace) ftrace_cpu_now_ts_end[5] 282906925000 info (trace) ftrace_cpu_now_ts_end[6] 282906987000 info (trace) ftrace_cpu_now_ts_end[7] 282907048000 info (trace) ftrace_cpu_oldest_event_ts_begin[0] 0 info (trace) ftrace_cpu_oldest_event_ts_begin[1] 93304642000 info (trace) ftrace_cpu_oldest_event_ts_begin[2] 0 info (trace) ftrace_cpu_oldest_event_ts_begin[3] 93304876000 info (trace) ftrace_cpu_oldest_event_ts_begin[4] 0 info (trace) ftrace_cpu_oldest_event_ts_begin[5] 0 info (trace) ftrace_cpu_oldest_event_ts_begin[6] 0 info (trace) ftrace_cpu_oldest_event_ts_begin[7] 0 info (trace) ftrace_cpu_oldest_event_ts_end[0] 282905715000 info (trace) ftrace_cpu_oldest_event_ts_end[1] 282903723000 info (trace) ftrace_cpu_oldest_event_ts_end[2] 282903881000 info (trace) ftrace_cpu_oldest_event_ts_end[3] 282816175000 info (trace) ftrace_cpu_oldest_event_ts_end[4] 282896619000 info (trace) ftrace_cpu_oldest_event_ts_end[5] 282884168000 info (trace) ftrace_cpu_oldest_event_ts_end[6] 282783221000 info (trace) ftrace_cpu_oldest_event_ts_end[7] 282880081000 info (trace) ftrace_cpu_overrun_begin[0] 0 info (trace) ftrace_cpu_overrun_begin[1] 0 info (trace) ftrace_cpu_overrun_begin[2] 0 info (trace) ftrace_cpu_overrun_begin[3] 0 info (trace) ftrace_cpu_overrun_begin[4] 0 info (trace) ftrace_cpu_overrun_begin[5] 0 info (trace) ftrace_cpu_overrun_begin[6] 0 info (trace) ftrace_cpu_overrun_begin[7] 0 info (trace) ftrace_cpu_overrun_delta[0]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[1]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[2]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[3]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[4]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[5]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[6]help_outline 0 data_loss (trace) ftrace_cpu_overrun_delta[7]help_outline 0 data_loss (trace) ftrace_cpu_overrun_end[0] 0 info (trace) ftrace_cpu_overrun_end[1] 0 info (trace) ftrace_cpu_overrun_end[2] 0 info (trace) ftrace_cpu_overrun_end[3] 0 info (trace) ftrace_cpu_overrun_end[4] 0 info (trace) ftrace_cpu_overrun_end[5] 0 info (trace) ftrace_cpu_overrun_end[6] 0 info (trace) ftrace_cpu_overrun_end[7] 0 info (trace) ftrace_cpu_read_events_begin[0] 0 info (trace) ftrace_cpu_read_events_begin[1] 0 info (trace) ftrace_cpu_read_events_begin[2] 0 info (trace) ftrace_cpu_read_events_begin[3] 0 info (trace) ftrace_cpu_read_events_begin[4] 0 info (trace) ftrace_cpu_read_events_begin[5] 0 info (trace) ftrace_cpu_read_events_begin[6] 0 info (trace) ftrace_cpu_read_events_begin[7] 0 info (trace) ftrace_cpu_read_events_delta[0] 454848 info (trace) ftrace_cpu_read_events_delta[1] 453484 info (trace) ftrace_cpu_read_events_delta[2] 386290 info (trace) ftrace_cpu_read_events_delta[3] 356432 info (trace) ftrace_cpu_read_events_delta[4] 393337 info (trace) ftrace_cpu_read_events_delta[5] 325244 info (trace) ftrace_cpu_read_events_delta[6] 392637 info (trace) ftrace_cpu_read_events_delta[7] 350623 info (trace) ftrace_cpu_read_events_end[0] 454848 info (trace) ftrace_cpu_read_events_end[1] 453484 info (trace) ftrace_cpu_read_events_end[2] 386290 info (trace) ftrace_cpu_read_events_end[3] 356432 info (trace) ftrace_cpu_read_events_end[4] 393337 info (trace) ftrace_cpu_read_events_end[5] 325244 info (trace) ftrace_cpu_read_events_end[6] 392637 info (trace) ftrace_cpu_read_events_end[7] 350623 info (trace) ftrace_packet_before_tracing_starthelp_outline 0 info (analysis) ftrace_setup_errorshelp_outline 0 error (trace) fuchsia_invalid_event 0 error (analysis) fuchsia_non_numeric_counters 0 error (analysis) fuchsia_timestamp_overflow 0 error (analysis) game_intervention_has_parse_errorshelp_outline 0 error (trace) game_intervention_has_read_errorshelp_outline 0 error (trace) gpu_counters_invalid_spec 0 error (analysis) gpu_counters_missing_spec 0 error (analysis) gpu_render_stage_parser_errors 0 error (analysis) graphics_frame_event_parser_errors 0 info (analysis) guess_trace_type_duration_ns 7654 info (analysis) heap_graph_non_finalized_graph 0 error (trace) heapprofd_missing_packet 0 error (trace) heapprofd_non_finalized_profile 0 error (trace) interned_data_tokenizer_errors 0 info (analysis) invalid_clock_snapshots 0 error (analysis) invalid_cpu_times 0 error (analysis) json_display_time_unithelp_outline 0 info (trace) json_parser_failure 0 error (trace) json_tokenizer_failure 0 error (trace) meminfo_unknown_keys 0 error (analysis) memory_snapshot_parser_failure 0 error (analysis) metatrace_overruns 0 error (trace) mismatched_sched_switch_tids 11101 error (analysis) misplaced_end_event 0 data_loss (analysis) mm_unknown_type 0 error (analysis) ninja_parse_errors 0 error (trace) packages_list_has_parse_errors 0 error (trace) packages_list_has_read_errors 0 error (trace) parse_trace_duration_ns 1780589548 info (analysis) perf_samples_skipped 0 info (trace) perf_samples_skipped_dataloss 0 data_loss (trace) power_rail_unknown_index 0 error (trace) proc_stat_unknown_counters 0 error (analysis) process_tracker_errors 0 error (analysis) rss_stat_negative_size 0 info (analysis) rss_stat_unknown_keys 0 error (analysis) rss_stat_unknown_thread_for_mm_id 0 info (analysis) sched_switch_out_of_order 0 error (analysis) sched_waking_out_of_order 0 error (analysis) slice_out_of_order 0 error (analysis) sorter_push_event_out_of_orderhelp_outline 0 error (trace) stackprofile_invalid_callstack_id 0 error (trace) stackprofile_invalid_frame_id 0 error (trace) stackprofile_invalid_mapping_id 0 error (trace) stackprofile_invalid_string_id 0 error (trace) stackprofile_parser_error 0 error (trace) symbolization_tmp_build_id_not_foundhelp_outline 0 error (analysis) systrace_parse_failure 19040 error (analysis) task_state_invalid 0 error (analysis) thread_time_in_state_out_of_order 0 error (analysis) thread_time_in_state_unknown_cpu_freq 0 error (analysis) tokenizer_skipped_packets 0 info (analysis) traced_buf_abi_violations[0] 0 data_loss (trace) traced_buf_abi_violations[1] 0 data_loss (trace) traced_buf_buffer_size[0] 534773760 info (trace) traced_buf_buffer_size[1] 2097152 info (trace) traced_buf_bytes_overwritten[0] 0 info (trace) traced_buf_bytes_overwritten[1] 0 info (trace) traced_buf_bytes_read[0] 78929920 info (trace) traced_buf_bytes_read[1] 425984 info (trace) traced_buf_bytes_written[0] 78962688 info (trace) traced_buf_bytes_written[1] 425984 info (trace) traced_buf_chunks_committed_out_of_order[0] 0 info (trace) traced_buf_chunks_committed_out_of_order[1] 0 info (trace) traced_buf_chunks_discarded[0] 0 info (trace) traced_buf_chunks_discarded[1] 0 info (trace) traced_buf_chunks_overwritten[0] 0 info (trace) traced_buf_chunks_overwritten[1] 0 info (trace) traced_buf_chunks_read[0] 2428 info (trace) traced_buf_chunks_read[1] 13 info (trace) traced_buf_chunks_rewritten[0] 6 info (trace) traced_buf_chunks_rewritten[1] 0 info (trace) traced_buf_chunks_written[0] 2429 info (trace) traced_buf_chunks_written[1] 13 info (trace) traced_buf_padding_bytes_cleared[0] 0 info (trace) traced_buf_padding_bytes_cleared[1] 0 info (trace) traced_buf_padding_bytes_written[0] 0 info (trace) traced_buf_padding_bytes_written[1] 0 info (trace) traced_buf_patches_failed[0] 0 data_loss (trace) traced_buf_patches_failed[1] 0 data_loss (trace) traced_buf_patches_succeeded[0] 5633 info (trace) traced_buf_patches_succeeded[1] 8 info (trace) traced_buf_readaheads_failed[0] 115 info (trace) traced_buf_readaheads_failed[1] 18 info (trace) traced_buf_readaheads_succeeded[0] 2257 info (trace) traced_buf_readaheads_succeeded[1] 6 info (trace) traced_buf_trace_writer_packet_loss[0] 0 data_loss (trace) traced_buf_trace_writer_packet_loss[1] 0 data_loss (trace) traced_buf_write_wrap_count[0] 0 info (trace) traced_buf_write_wrap_count[1] 0 info (trace) traced_chunks_discarded 0 info (trace) traced_data_sources_registered 16 info (trace) traced_data_sources_seen 6 info (trace) traced_final_flush_failed 0 data_loss (trace) traced_final_flush_succeeded 0 info (trace) traced_flushes_failed 0 data_loss (trace) traced_flushes_requested 0 info (trace) traced_flushes_succeeded 0 info (trace) traced_patches_discarded 0 info (trace) traced_producers_connected 3 info (trace) traced_producers_seen 3 info (trace) traced_total_buffers 2 info (trace) traced_tracing_sessions 1 info (trace) track_event_dropped_packets_outside_of_range_of_interesthelp_outline 0 info (analysis) track_event_parser_errors 0 info (analysis) track_event_thread_invalid_endhelp_outline 0 error (trace) track_event_tokenizer_errors 0 info (analysis) truncated_sys_write_durationhelp_outline 0 data_loss (analysis) unknown_extension_fieldshelp_outline 0 error (trace) vmstat_unknown_keys 0 error (analysis) vulkan_allocations_invalid_string_id 0 error (trace) > -- > Regards > Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 14:27 ` Kajetan Puchalski @ 2023-02-09 15:37 ` Yafang Shao 2023-02-10 18:09 ` Kajetan Puchalski 2023-02-11 16:51 ` Qais Yousef 0 siblings, 2 replies; 43+ messages in thread From: Yafang Shao @ 2023-02-09 15:37 UTC (permalink / raw) To: Kajetan Puchalski Cc: Mathieu Desnoyers, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Thu, Feb 9, 2023 at 10:28 PM Kajetan Puchalski <kajetan.puchalski@arm.com> wrote: > > On Thu, Feb 09, 2023 at 02:20:36PM +0800, Yafang Shao wrote: > > [...] > > Hi Yafang, > > > Many thanks for the detailed analysis. Seems it can work. > > > > Hi John, > > > > Could you pls. try the attached fix ? I have verified it in my test env. > > I tested the patch on my environment where I found the issue with newer > kernels + older Perfetto. The patch does improve things so that's nice. Thanks for the test. I don't have Perfetto in hand, so I haven't verify Perfetto. > It goes from "not working at all" to "mostly working but missing data" > compared to what happens if I just revert 3087c61ed2c48548b74dd343a5209b87082c682d. > Do you mean there are no errors at all if revert 3087c61ed2c48548b74dd343a5209b87082c682d ? > I'm just an end user so can't really speak to the underlying causes but > for those more familiar with how Perfetto works this is what I'm getting: > The sched_switch tracepoint format file has the same output with reverting the commit, $ cat /sys/kernel/debug/tracing/events/sched/sched_switch/format name: sched_switch ID: 286 format: field:unsigned short common_type; offset:0; size:2; signed:0; field:unsigned char common_flags; offset:2; size:1; signed:0; field:unsigned char common_preempt_count; offset:3; size:1; signed:0; field:int common_pid; offset:4; size:4; signed:1; field:char prev_comm[16]; offset:8; size:16; signed:0; field:pid_t prev_pid; offset:24; size:4; signed:1; field:int prev_prio; offset:28; size:4; signed:1; field:long prev_state; offset:32; size:8; signed:1; field:char next_comm[16]; offset:40; size:16; signed:0; field:pid_t next_pid; offset:56; size:4; signed:1; field:int next_prio; offset:60; size:4; signed:1; print fmt: "prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d", REC->prev_comm, REC->prev_pid, REC->prev_prio, (REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1)) ? __print_flags(REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1), "|", { 0x00000001, "S" }, { 0x00000002, "D" }, { 0x00000004, "T" }, { 0x00000008, "t" }, { 0x00000010, "X" }, { 0x00000020, "Z" }, { 0x00000040, "P" }, { 0x00000080, "I" }) : "R", REC->prev_state & (((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) ? "+" : "", REC->next_comm, REC->next_pid, REC->next_prio So may be these errors were caused by other issues ? > Error stats for this trace: > name idx source value > ---------------------------------------- ---------------------------------------- ---------------------------------------- ---------------------------------------- > mismatched_sched_switch_tids [NULL] analysis 11101 > systrace_parse_failure [NULL] analysis 19040 > > The trace explorer window ends up containing the ftrace-specific tracks > but missing the tracks related to Android-specific callbacks and such. > > Debug stats below in case they're relevant: > > Name Value Type > android_br_parse_errors 0 error (trace) > android_log_format_invalid 0 error (trace) > android_log_num_failed 0 error (trace) > android_log_num_skipped 0 info (trace) > android_log_num_total 0 info (trace) > clock_sync_cache_miss 181 info (analysis) > clock_sync_failure 0 error (analysis) > compact_sched_has_parse_errors 0 error (trace) > compact_sched_switch_skipped 0 info (analysis) > compact_sched_waking_skipped 0 info (analysis) > counter_events_out_of_order 0 error (analysis) > deobfuscate_location_parse_error 0 error (trace) > empty_chrome_metadata 0 error (trace) > energy_breakdown_missing_values 0 error (analysis) > energy_descriptor_invalid 0 error (analysis) > energy_uid_breakdown_missing_values 0 error (analysis) > flow_duplicate_id 0 error (trace) > flow_end_without_start 0 info (trace) > flow_invalid_id 0 error (trace) > flow_no_enclosing_slice 0 error (trace) > flow_step_without_start 0 info (trace) > flow_without_direction 0 error (trace) > frame_timeline_event_parser_errors 0 info (analysis) > ftrace_bundle_tokenizer_errors 0 error (analysis) > ftrace_cpu_bytes_read_begin[0] 0 info (trace) > ftrace_cpu_bytes_read_begin[1] 264 info (trace) > ftrace_cpu_bytes_read_begin[2] 0 info (trace) > ftrace_cpu_bytes_read_begin[3] 224 info (trace) > ftrace_cpu_bytes_read_begin[4] 0 info (trace) > ftrace_cpu_bytes_read_begin[5] 0 info (trace) > ftrace_cpu_bytes_read_begin[6] 0 info (trace) > ftrace_cpu_bytes_read_begin[7] 0 info (trace) > ftrace_cpu_bytes_read_delta[0] 6919836 info (trace) > ftrace_cpu_bytes_read_delta[1] 7197556 info (trace) > ftrace_cpu_bytes_read_delta[2] 6381828 info (trace) > ftrace_cpu_bytes_read_delta[3] 5988336 info (trace) > ftrace_cpu_bytes_read_delta[4] 5933528 info (trace) > ftrace_cpu_bytes_read_delta[5] 4858400 info (trace) > ftrace_cpu_bytes_read_delta[6] 6175260 info (trace) > ftrace_cpu_bytes_read_delta[7] 4633460 info (trace) > ftrace_cpu_bytes_read_end[0] 6919836 info (trace) > ftrace_cpu_bytes_read_end[1] 7197820 info (trace) > ftrace_cpu_bytes_read_end[2] 6381828 info (trace) > ftrace_cpu_bytes_read_end[3] 5988560 info (trace) > ftrace_cpu_bytes_read_end[4] 5933528 info (trace) > ftrace_cpu_bytes_read_end[5] 4858400 info (trace) > ftrace_cpu_bytes_read_end[6] 6175260 info (trace) > ftrace_cpu_bytes_read_end[7] 4633460 info (trace) > ftrace_cpu_commit_overrun_begin[0] 0 info (trace) > ftrace_cpu_commit_overrun_begin[1] 0 info (trace) > ftrace_cpu_commit_overrun_begin[2] 0 info (trace) > ftrace_cpu_commit_overrun_begin[3] 0 info (trace) > ftrace_cpu_commit_overrun_begin[4] 0 info (trace) > ftrace_cpu_commit_overrun_begin[5] 0 info (trace) > ftrace_cpu_commit_overrun_begin[6] 0 info (trace) > ftrace_cpu_commit_overrun_begin[7] 0 info (trace) > ftrace_cpu_commit_overrun_delta[0] 0 error (trace) > ftrace_cpu_commit_overrun_delta[1] 0 error (trace) > ftrace_cpu_commit_overrun_delta[2] 0 error (trace) > ftrace_cpu_commit_overrun_delta[3] 0 error (trace) > ftrace_cpu_commit_overrun_delta[4] 0 error (trace) > ftrace_cpu_commit_overrun_delta[5] 0 error (trace) > ftrace_cpu_commit_overrun_delta[6] 0 error (trace) > ftrace_cpu_commit_overrun_delta[7] 0 error (trace) > ftrace_cpu_commit_overrun_end[0] 0 info (trace) > ftrace_cpu_commit_overrun_end[1] 0 info (trace) > ftrace_cpu_commit_overrun_end[2] 0 info (trace) > ftrace_cpu_commit_overrun_end[3] 0 info (trace) > ftrace_cpu_commit_overrun_end[4] 0 info (trace) > ftrace_cpu_commit_overrun_end[5] 0 info (trace) > ftrace_cpu_commit_overrun_end[6] 0 info (trace) > ftrace_cpu_commit_overrun_end[7] 0 info (trace) > ftrace_cpu_dropped_events_begin[0] 0 info (trace) > ftrace_cpu_dropped_events_begin[1] 0 info (trace) > ftrace_cpu_dropped_events_begin[2] 0 info (trace) > ftrace_cpu_dropped_events_begin[3] 0 info (trace) > ftrace_cpu_dropped_events_begin[4] 0 info (trace) > ftrace_cpu_dropped_events_begin[5] 0 info (trace) > ftrace_cpu_dropped_events_begin[6] 0 info (trace) > ftrace_cpu_dropped_events_begin[7] 0 info (trace) > ftrace_cpu_dropped_events_delta[0] 0 error (trace) > ftrace_cpu_dropped_events_delta[1] 0 error (trace) > ftrace_cpu_dropped_events_delta[2] 0 error (trace) > ftrace_cpu_dropped_events_delta[3] 0 error (trace) > ftrace_cpu_dropped_events_delta[4] 0 error (trace) > ftrace_cpu_dropped_events_delta[5] 0 error (trace) > ftrace_cpu_dropped_events_delta[6] 0 error (trace) > ftrace_cpu_dropped_events_delta[7] 0 error (trace) > ftrace_cpu_dropped_events_end[0] 0 info (trace) > ftrace_cpu_dropped_events_end[1] 0 info (trace) > ftrace_cpu_dropped_events_end[2] 0 info (trace) > ftrace_cpu_dropped_events_end[3] 0 info (trace) > ftrace_cpu_dropped_events_end[4] 0 info (trace) > ftrace_cpu_dropped_events_end[5] 0 info (trace) > ftrace_cpu_dropped_events_end[6] 0 info (trace) > ftrace_cpu_dropped_events_end[7] 0 info (trace) > ftrace_cpu_entries_begin[0] 0 info (trace) > ftrace_cpu_entries_begin[1] 6 info (trace) > ftrace_cpu_entries_begin[2] 0 info (trace) > ftrace_cpu_entries_begin[3] 5 info (trace) > ftrace_cpu_entries_begin[4] 0 info (trace) > ftrace_cpu_entries_begin[5] 0 info (trace) > ftrace_cpu_entries_begin[6] 0 info (trace) > ftrace_cpu_entries_begin[7] 0 info (trace) > ftrace_cpu_entries_delta[0] 6 info (trace) > ftrace_cpu_entries_delta[1] -6 info (trace) > ftrace_cpu_entries_delta[2] 0 info (trace) > ftrace_cpu_entries_delta[3] 2 info (trace) > ftrace_cpu_entries_delta[4] 0 info (trace) > ftrace_cpu_entries_delta[5] 0 info (trace) > ftrace_cpu_entries_delta[6] 0 info (trace) > ftrace_cpu_entries_delta[7] 0 info (trace) > ftrace_cpu_entries_end[0] 6 info (trace) > ftrace_cpu_entries_end[1] 0 info (trace) > ftrace_cpu_entries_end[2] 0 info (trace) > ftrace_cpu_entries_end[3] 7 info (trace) > ftrace_cpu_entries_end[4] 0 info (trace) > ftrace_cpu_entries_end[5] 0 info (trace) > ftrace_cpu_entries_end[6] 0 info (trace) > ftrace_cpu_entries_end[7] 0 info (trace) > ftrace_cpu_now_ts_begin[0] 93305027000 info (trace) > ftrace_cpu_now_ts_begin[1] 93305103000 info (trace) > ftrace_cpu_now_ts_begin[2] 93305159000 info (trace) > ftrace_cpu_now_ts_begin[3] 93305207000 info (trace) > ftrace_cpu_now_ts_begin[4] 93305262000 info (trace) > ftrace_cpu_now_ts_begin[5] 93305312000 info (trace) > ftrace_cpu_now_ts_begin[6] 93305362000 info (trace) > ftrace_cpu_now_ts_begin[7] 93305411000 info (trace) > ftrace_cpu_now_ts_end[0] 282906571000 info (trace) > ftrace_cpu_now_ts_end[1] 282906676000 info (trace) > ftrace_cpu_now_ts_end[2] 282906738000 info (trace) > ftrace_cpu_now_ts_end[3] 282906803000 info (trace) > ftrace_cpu_now_ts_end[4] 282906863000 info (trace) > ftrace_cpu_now_ts_end[5] 282906925000 info (trace) > ftrace_cpu_now_ts_end[6] 282906987000 info (trace) > ftrace_cpu_now_ts_end[7] 282907048000 info (trace) > ftrace_cpu_oldest_event_ts_begin[0] 0 info (trace) > ftrace_cpu_oldest_event_ts_begin[1] 93304642000 info (trace) > ftrace_cpu_oldest_event_ts_begin[2] 0 info (trace) > ftrace_cpu_oldest_event_ts_begin[3] 93304876000 info (trace) > ftrace_cpu_oldest_event_ts_begin[4] 0 info (trace) > ftrace_cpu_oldest_event_ts_begin[5] 0 info (trace) > ftrace_cpu_oldest_event_ts_begin[6] 0 info (trace) > ftrace_cpu_oldest_event_ts_begin[7] 0 info (trace) > ftrace_cpu_oldest_event_ts_end[0] 282905715000 info (trace) > ftrace_cpu_oldest_event_ts_end[1] 282903723000 info (trace) > ftrace_cpu_oldest_event_ts_end[2] 282903881000 info (trace) > ftrace_cpu_oldest_event_ts_end[3] 282816175000 info (trace) > ftrace_cpu_oldest_event_ts_end[4] 282896619000 info (trace) > ftrace_cpu_oldest_event_ts_end[5] 282884168000 info (trace) > ftrace_cpu_oldest_event_ts_end[6] 282783221000 info (trace) > ftrace_cpu_oldest_event_ts_end[7] 282880081000 info (trace) > ftrace_cpu_overrun_begin[0] 0 info (trace) > ftrace_cpu_overrun_begin[1] 0 info (trace) > ftrace_cpu_overrun_begin[2] 0 info (trace) > ftrace_cpu_overrun_begin[3] 0 info (trace) > ftrace_cpu_overrun_begin[4] 0 info (trace) > ftrace_cpu_overrun_begin[5] 0 info (trace) > ftrace_cpu_overrun_begin[6] 0 info (trace) > ftrace_cpu_overrun_begin[7] 0 info (trace) > ftrace_cpu_overrun_delta[0]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[1]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[2]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[3]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[4]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[5]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[6]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_delta[7]help_outline 0 data_loss (trace) > ftrace_cpu_overrun_end[0] 0 info (trace) > ftrace_cpu_overrun_end[1] 0 info (trace) > ftrace_cpu_overrun_end[2] 0 info (trace) > ftrace_cpu_overrun_end[3] 0 info (trace) > ftrace_cpu_overrun_end[4] 0 info (trace) > ftrace_cpu_overrun_end[5] 0 info (trace) > ftrace_cpu_overrun_end[6] 0 info (trace) > ftrace_cpu_overrun_end[7] 0 info (trace) > ftrace_cpu_read_events_begin[0] 0 info (trace) > ftrace_cpu_read_events_begin[1] 0 info (trace) > ftrace_cpu_read_events_begin[2] 0 info (trace) > ftrace_cpu_read_events_begin[3] 0 info (trace) > ftrace_cpu_read_events_begin[4] 0 info (trace) > ftrace_cpu_read_events_begin[5] 0 info (trace) > ftrace_cpu_read_events_begin[6] 0 info (trace) > ftrace_cpu_read_events_begin[7] 0 info (trace) > ftrace_cpu_read_events_delta[0] 454848 info (trace) > ftrace_cpu_read_events_delta[1] 453484 info (trace) > ftrace_cpu_read_events_delta[2] 386290 info (trace) > ftrace_cpu_read_events_delta[3] 356432 info (trace) > ftrace_cpu_read_events_delta[4] 393337 info (trace) > ftrace_cpu_read_events_delta[5] 325244 info (trace) > ftrace_cpu_read_events_delta[6] 392637 info (trace) > ftrace_cpu_read_events_delta[7] 350623 info (trace) > ftrace_cpu_read_events_end[0] 454848 info (trace) > ftrace_cpu_read_events_end[1] 453484 info (trace) > ftrace_cpu_read_events_end[2] 386290 info (trace) > ftrace_cpu_read_events_end[3] 356432 info (trace) > ftrace_cpu_read_events_end[4] 393337 info (trace) > ftrace_cpu_read_events_end[5] 325244 info (trace) > ftrace_cpu_read_events_end[6] 392637 info (trace) > ftrace_cpu_read_events_end[7] 350623 info (trace) > ftrace_packet_before_tracing_starthelp_outline 0 info (analysis) > ftrace_setup_errorshelp_outline 0 error (trace) > fuchsia_invalid_event 0 error (analysis) > fuchsia_non_numeric_counters 0 error (analysis) > fuchsia_timestamp_overflow 0 error (analysis) > game_intervention_has_parse_errorshelp_outline 0 error (trace) > game_intervention_has_read_errorshelp_outline 0 error (trace) > gpu_counters_invalid_spec 0 error (analysis) > gpu_counters_missing_spec 0 error (analysis) > gpu_render_stage_parser_errors 0 error (analysis) > graphics_frame_event_parser_errors 0 info (analysis) > guess_trace_type_duration_ns 7654 info (analysis) > heap_graph_non_finalized_graph 0 error (trace) > heapprofd_missing_packet 0 error (trace) > heapprofd_non_finalized_profile 0 error (trace) > interned_data_tokenizer_errors 0 info (analysis) > invalid_clock_snapshots 0 error (analysis) > invalid_cpu_times 0 error (analysis) > json_display_time_unithelp_outline 0 info (trace) > json_parser_failure 0 error (trace) > json_tokenizer_failure 0 error (trace) > meminfo_unknown_keys 0 error (analysis) > memory_snapshot_parser_failure 0 error (analysis) > metatrace_overruns 0 error (trace) > mismatched_sched_switch_tids 11101 error (analysis) > misplaced_end_event 0 data_loss (analysis) > mm_unknown_type 0 error (analysis) > ninja_parse_errors 0 error (trace) > packages_list_has_parse_errors 0 error (trace) > packages_list_has_read_errors 0 error (trace) > parse_trace_duration_ns 1780589548 info (analysis) > perf_samples_skipped 0 info (trace) > perf_samples_skipped_dataloss 0 data_loss (trace) > power_rail_unknown_index 0 error (trace) > proc_stat_unknown_counters 0 error (analysis) > process_tracker_errors 0 error (analysis) > rss_stat_negative_size 0 info (analysis) > rss_stat_unknown_keys 0 error (analysis) > rss_stat_unknown_thread_for_mm_id 0 info (analysis) > sched_switch_out_of_order 0 error (analysis) > sched_waking_out_of_order 0 error (analysis) > slice_out_of_order 0 error (analysis) > sorter_push_event_out_of_orderhelp_outline 0 error (trace) > stackprofile_invalid_callstack_id 0 error (trace) > stackprofile_invalid_frame_id 0 error (trace) > stackprofile_invalid_mapping_id 0 error (trace) > stackprofile_invalid_string_id 0 error (trace) > stackprofile_parser_error 0 error (trace) > symbolization_tmp_build_id_not_foundhelp_outline 0 error (analysis) > systrace_parse_failure 19040 error (analysis) > task_state_invalid 0 error (analysis) > thread_time_in_state_out_of_order 0 error (analysis) > thread_time_in_state_unknown_cpu_freq 0 error (analysis) > tokenizer_skipped_packets 0 info (analysis) > traced_buf_abi_violations[0] 0 data_loss (trace) > traced_buf_abi_violations[1] 0 data_loss (trace) > traced_buf_buffer_size[0] 534773760 info (trace) > traced_buf_buffer_size[1] 2097152 info (trace) > traced_buf_bytes_overwritten[0] 0 info (trace) > traced_buf_bytes_overwritten[1] 0 info (trace) > traced_buf_bytes_read[0] 78929920 info (trace) > traced_buf_bytes_read[1] 425984 info (trace) > traced_buf_bytes_written[0] 78962688 info (trace) > traced_buf_bytes_written[1] 425984 info (trace) > traced_buf_chunks_committed_out_of_order[0] 0 info (trace) > traced_buf_chunks_committed_out_of_order[1] 0 info (trace) > traced_buf_chunks_discarded[0] 0 info (trace) > traced_buf_chunks_discarded[1] 0 info (trace) > traced_buf_chunks_overwritten[0] 0 info (trace) > traced_buf_chunks_overwritten[1] 0 info (trace) > traced_buf_chunks_read[0] 2428 info (trace) > traced_buf_chunks_read[1] 13 info (trace) > traced_buf_chunks_rewritten[0] 6 info (trace) > traced_buf_chunks_rewritten[1] 0 info (trace) > traced_buf_chunks_written[0] 2429 info (trace) > traced_buf_chunks_written[1] 13 info (trace) > traced_buf_padding_bytes_cleared[0] 0 info (trace) > traced_buf_padding_bytes_cleared[1] 0 info (trace) > traced_buf_padding_bytes_written[0] 0 info (trace) > traced_buf_padding_bytes_written[1] 0 info (trace) > traced_buf_patches_failed[0] 0 data_loss (trace) > traced_buf_patches_failed[1] 0 data_loss (trace) > traced_buf_patches_succeeded[0] 5633 info (trace) > traced_buf_patches_succeeded[1] 8 info (trace) > traced_buf_readaheads_failed[0] 115 info (trace) > traced_buf_readaheads_failed[1] 18 info (trace) > traced_buf_readaheads_succeeded[0] 2257 info (trace) > traced_buf_readaheads_succeeded[1] 6 info (trace) > traced_buf_trace_writer_packet_loss[0] 0 data_loss (trace) > traced_buf_trace_writer_packet_loss[1] 0 data_loss (trace) > traced_buf_write_wrap_count[0] 0 info (trace) > traced_buf_write_wrap_count[1] 0 info (trace) > traced_chunks_discarded 0 info (trace) > traced_data_sources_registered 16 info (trace) > traced_data_sources_seen 6 info (trace) > traced_final_flush_failed 0 data_loss (trace) > traced_final_flush_succeeded 0 info (trace) > traced_flushes_failed 0 data_loss (trace) > traced_flushes_requested 0 info (trace) > traced_flushes_succeeded 0 info (trace) > traced_patches_discarded 0 info (trace) > traced_producers_connected 3 info (trace) > traced_producers_seen 3 info (trace) > traced_total_buffers 2 info (trace) > traced_tracing_sessions 1 info (trace) > track_event_dropped_packets_outside_of_range_of_interesthelp_outline 0 info (analysis) > track_event_parser_errors 0 info (analysis) > track_event_thread_invalid_endhelp_outline 0 error (trace) > track_event_tokenizer_errors 0 info (analysis) > truncated_sys_write_durationhelp_outline 0 data_loss (analysis) > unknown_extension_fieldshelp_outline 0 error (trace) > vmstat_unknown_keys 0 error (analysis) > vulkan_allocations_invalid_string_id 0 error (trace) > > > -- > > Regards > > Yafang > > -- Regards Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 15:37 ` Yafang Shao @ 2023-02-10 18:09 ` Kajetan Puchalski 2023-02-11 16:51 ` Qais Yousef 1 sibling, 0 replies; 43+ messages in thread From: Kajetan Puchalski @ 2023-02-10 18:09 UTC (permalink / raw) To: Yafang Shao Cc: Mathieu Desnoyers, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Thu, Feb 09, 2023 at 11:37:44PM +0800, Yafang Shao wrote: > > > It goes from "not working at all" to "mostly working but missing data" > > compared to what happens if I just revert 3087c61ed2c48548b74dd343a5209b87082c682d. > > > > Do you mean there are no errors at all if revert > 3087c61ed2c48548b74dd343a5209b87082c682d ? Correct yes, the revert makes it work perfectly. > > I'm just an end user so can't really speak to the underlying causes but > > for those more familiar with how Perfetto works this is what I'm getting: > > > > The sched_switch tracepoint format file has the same output with > reverting the commit, > > $ cat /sys/kernel/debug/tracing/events/sched/sched_switch/format > name: sched_switch > ID: 286 > format: > field:unsigned short common_type; offset:0; size:2; signed:0; > field:unsigned char common_flags; offset:2; size:1; signed:0; > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > field:int common_pid; offset:4; size:4; signed:1; > field:char prev_comm[16]; offset:8; size:16; signed:0; > field:pid_t prev_pid; offset:24; size:4; signed:1; > field:int prev_prio; offset:28; size:4; signed:1; > field:long prev_state; offset:32; size:8; signed:1; > field:char next_comm[16]; offset:40; size:16; signed:0; > field:pid_t next_pid; offset:56; size:4; signed:1; > field:int next_prio; offset:60; size:4; signed:1; > > print fmt: "prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> > next_comm=%s next_pid=%d next_prio=%d", REC->prev_comm, REC->prev_pid, > REC->prev_prio, (REC->prev_state & ((((0x00000000 | 0x00000001 | > 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | > 0x00000040) + 1) << 1) - 1)) ? __print_flags(REC->prev_state & > ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | > 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1), "|", { > 0x00000001, "S" }, { 0x00000002, "D" }, { 0x00000004, "T" }, { > 0x00000008, "t" }, { 0x00000010, "X" }, { 0x00000020, "Z" }, { > 0x00000040, "P" }, { 0x00000080, "I" }) : "R", REC->prev_state & > (((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | > 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) ? "+" : "", > REC->next_comm, REC->next_pid, REC->next_prio > > So may be these errors were caused by other issues ? As I said not really sure why it's happening but there's definitely an issue somewhere. I'm hoping someone more familiar with how Perfetto works might have an idea. > > Error stats for this trace: > > name idx source value > > ---------------------------------------- ---------------------------------------- ---------------------------------------- ---------------------------------------- > > mismatched_sched_switch_tids [NULL] analysis 11101 > > systrace_parse_failure [NULL] analysis 19040 > > > > The trace explorer window ends up containing the ftrace-specific tracks > > but missing the tracks related to Android-specific callbacks and such. > > > > Debug stats below in case they're relevant: > > > > Name Value Type > > android_br_parse_errors 0 error (trace) > > android_log_format_invalid 0 error (trace) > > android_log_num_failed 0 error (trace) > > android_log_num_skipped 0 info (trace) > > android_log_num_total 0 info (trace) > > clock_sync_cache_miss 181 info (analysis) > > clock_sync_failure 0 error (analysis) > > compact_sched_has_parse_errors 0 error (trace) > > compact_sched_switch_skipped 0 info (analysis) > > compact_sched_waking_skipped 0 info (analysis) > > counter_events_out_of_order 0 error (analysis) > > deobfuscate_location_parse_error 0 error (trace) > > empty_chrome_metadata 0 error (trace) > > energy_breakdown_missing_values 0 error (analysis) > > energy_descriptor_invalid 0 error (analysis) > > energy_uid_breakdown_missing_values 0 error (analysis) > > flow_duplicate_id 0 error (trace) > > flow_end_without_start 0 info (trace) > > flow_invalid_id 0 error (trace) > > flow_no_enclosing_slice 0 error (trace) > > flow_step_without_start 0 info (trace) > > flow_without_direction 0 error (trace) > > frame_timeline_event_parser_errors 0 info (analysis) > > ftrace_bundle_tokenizer_errors 0 error (analysis) > > ftrace_cpu_bytes_read_begin[0] 0 info (trace) > > ftrace_cpu_bytes_read_begin[1] 264 info (trace) > > ftrace_cpu_bytes_read_begin[2] 0 info (trace) > > ftrace_cpu_bytes_read_begin[3] 224 info (trace) > > ftrace_cpu_bytes_read_begin[4] 0 info (trace) > > ftrace_cpu_bytes_read_begin[5] 0 info (trace) > > ftrace_cpu_bytes_read_begin[6] 0 info (trace) > > ftrace_cpu_bytes_read_begin[7] 0 info (trace) > > ftrace_cpu_bytes_read_delta[0] 6919836 info (trace) > > ftrace_cpu_bytes_read_delta[1] 7197556 info (trace) > > ftrace_cpu_bytes_read_delta[2] 6381828 info (trace) > > ftrace_cpu_bytes_read_delta[3] 5988336 info (trace) > > ftrace_cpu_bytes_read_delta[4] 5933528 info (trace) > > ftrace_cpu_bytes_read_delta[5] 4858400 info (trace) > > ftrace_cpu_bytes_read_delta[6] 6175260 info (trace) > > ftrace_cpu_bytes_read_delta[7] 4633460 info (trace) > > ftrace_cpu_bytes_read_end[0] 6919836 info (trace) > > ftrace_cpu_bytes_read_end[1] 7197820 info (trace) > > ftrace_cpu_bytes_read_end[2] 6381828 info (trace) > > ftrace_cpu_bytes_read_end[3] 5988560 info (trace) > > ftrace_cpu_bytes_read_end[4] 5933528 info (trace) > > ftrace_cpu_bytes_read_end[5] 4858400 info (trace) > > ftrace_cpu_bytes_read_end[6] 6175260 info (trace) > > ftrace_cpu_bytes_read_end[7] 4633460 info (trace) > > ftrace_cpu_commit_overrun_begin[0] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[1] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[2] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[3] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[4] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[5] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[6] 0 info (trace) > > ftrace_cpu_commit_overrun_begin[7] 0 info (trace) > > ftrace_cpu_commit_overrun_delta[0] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[1] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[2] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[3] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[4] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[5] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[6] 0 error (trace) > > ftrace_cpu_commit_overrun_delta[7] 0 error (trace) > > ftrace_cpu_commit_overrun_end[0] 0 info (trace) > > ftrace_cpu_commit_overrun_end[1] 0 info (trace) > > ftrace_cpu_commit_overrun_end[2] 0 info (trace) > > ftrace_cpu_commit_overrun_end[3] 0 info (trace) > > ftrace_cpu_commit_overrun_end[4] 0 info (trace) > > ftrace_cpu_commit_overrun_end[5] 0 info (trace) > > ftrace_cpu_commit_overrun_end[6] 0 info (trace) > > ftrace_cpu_commit_overrun_end[7] 0 info (trace) > > ftrace_cpu_dropped_events_begin[0] 0 info (trace) > > ftrace_cpu_dropped_events_begin[1] 0 info (trace) > > ftrace_cpu_dropped_events_begin[2] 0 info (trace) > > ftrace_cpu_dropped_events_begin[3] 0 info (trace) > > ftrace_cpu_dropped_events_begin[4] 0 info (trace) > > ftrace_cpu_dropped_events_begin[5] 0 info (trace) > > ftrace_cpu_dropped_events_begin[6] 0 info (trace) > > ftrace_cpu_dropped_events_begin[7] 0 info (trace) > > ftrace_cpu_dropped_events_delta[0] 0 error (trace) > > ftrace_cpu_dropped_events_delta[1] 0 error (trace) > > ftrace_cpu_dropped_events_delta[2] 0 error (trace) > > ftrace_cpu_dropped_events_delta[3] 0 error (trace) > > ftrace_cpu_dropped_events_delta[4] 0 error (trace) > > ftrace_cpu_dropped_events_delta[5] 0 error (trace) > > ftrace_cpu_dropped_events_delta[6] 0 error (trace) > > ftrace_cpu_dropped_events_delta[7] 0 error (trace) > > ftrace_cpu_dropped_events_end[0] 0 info (trace) > > ftrace_cpu_dropped_events_end[1] 0 info (trace) > > ftrace_cpu_dropped_events_end[2] 0 info (trace) > > ftrace_cpu_dropped_events_end[3] 0 info (trace) > > ftrace_cpu_dropped_events_end[4] 0 info (trace) > > ftrace_cpu_dropped_events_end[5] 0 info (trace) > > ftrace_cpu_dropped_events_end[6] 0 info (trace) > > ftrace_cpu_dropped_events_end[7] 0 info (trace) > > ftrace_cpu_entries_begin[0] 0 info (trace) > > ftrace_cpu_entries_begin[1] 6 info (trace) > > ftrace_cpu_entries_begin[2] 0 info (trace) > > ftrace_cpu_entries_begin[3] 5 info (trace) > > ftrace_cpu_entries_begin[4] 0 info (trace) > > ftrace_cpu_entries_begin[5] 0 info (trace) > > ftrace_cpu_entries_begin[6] 0 info (trace) > > ftrace_cpu_entries_begin[7] 0 info (trace) > > ftrace_cpu_entries_delta[0] 6 info (trace) > > ftrace_cpu_entries_delta[1] -6 info (trace) > > ftrace_cpu_entries_delta[2] 0 info (trace) > > ftrace_cpu_entries_delta[3] 2 info (trace) > > ftrace_cpu_entries_delta[4] 0 info (trace) > > ftrace_cpu_entries_delta[5] 0 info (trace) > > ftrace_cpu_entries_delta[6] 0 info (trace) > > ftrace_cpu_entries_delta[7] 0 info (trace) > > ftrace_cpu_entries_end[0] 6 info (trace) > > ftrace_cpu_entries_end[1] 0 info (trace) > > ftrace_cpu_entries_end[2] 0 info (trace) > > ftrace_cpu_entries_end[3] 7 info (trace) > > ftrace_cpu_entries_end[4] 0 info (trace) > > ftrace_cpu_entries_end[5] 0 info (trace) > > ftrace_cpu_entries_end[6] 0 info (trace) > > ftrace_cpu_entries_end[7] 0 info (trace) > > ftrace_cpu_now_ts_begin[0] 93305027000 info (trace) > > ftrace_cpu_now_ts_begin[1] 93305103000 info (trace) > > ftrace_cpu_now_ts_begin[2] 93305159000 info (trace) > > ftrace_cpu_now_ts_begin[3] 93305207000 info (trace) > > ftrace_cpu_now_ts_begin[4] 93305262000 info (trace) > > ftrace_cpu_now_ts_begin[5] 93305312000 info (trace) > > ftrace_cpu_now_ts_begin[6] 93305362000 info (trace) > > ftrace_cpu_now_ts_begin[7] 93305411000 info (trace) > > ftrace_cpu_now_ts_end[0] 282906571000 info (trace) > > ftrace_cpu_now_ts_end[1] 282906676000 info (trace) > > ftrace_cpu_now_ts_end[2] 282906738000 info (trace) > > ftrace_cpu_now_ts_end[3] 282906803000 info (trace) > > ftrace_cpu_now_ts_end[4] 282906863000 info (trace) > > ftrace_cpu_now_ts_end[5] 282906925000 info (trace) > > ftrace_cpu_now_ts_end[6] 282906987000 info (trace) > > ftrace_cpu_now_ts_end[7] 282907048000 info (trace) > > ftrace_cpu_oldest_event_ts_begin[0] 0 info (trace) > > ftrace_cpu_oldest_event_ts_begin[1] 93304642000 info (trace) > > ftrace_cpu_oldest_event_ts_begin[2] 0 info (trace) > > ftrace_cpu_oldest_event_ts_begin[3] 93304876000 info (trace) > > ftrace_cpu_oldest_event_ts_begin[4] 0 info (trace) > > ftrace_cpu_oldest_event_ts_begin[5] 0 info (trace) > > ftrace_cpu_oldest_event_ts_begin[6] 0 info (trace) > > ftrace_cpu_oldest_event_ts_begin[7] 0 info (trace) > > ftrace_cpu_oldest_event_ts_end[0] 282905715000 info (trace) > > ftrace_cpu_oldest_event_ts_end[1] 282903723000 info (trace) > > ftrace_cpu_oldest_event_ts_end[2] 282903881000 info (trace) > > ftrace_cpu_oldest_event_ts_end[3] 282816175000 info (trace) > > ftrace_cpu_oldest_event_ts_end[4] 282896619000 info (trace) > > ftrace_cpu_oldest_event_ts_end[5] 282884168000 info (trace) > > ftrace_cpu_oldest_event_ts_end[6] 282783221000 info (trace) > > ftrace_cpu_oldest_event_ts_end[7] 282880081000 info (trace) > > ftrace_cpu_overrun_begin[0] 0 info (trace) > > ftrace_cpu_overrun_begin[1] 0 info (trace) > > ftrace_cpu_overrun_begin[2] 0 info (trace) > > ftrace_cpu_overrun_begin[3] 0 info (trace) > > ftrace_cpu_overrun_begin[4] 0 info (trace) > > ftrace_cpu_overrun_begin[5] 0 info (trace) > > ftrace_cpu_overrun_begin[6] 0 info (trace) > > ftrace_cpu_overrun_begin[7] 0 info (trace) > > ftrace_cpu_overrun_delta[0]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[1]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[2]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[3]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[4]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[5]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[6]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_delta[7]help_outline 0 data_loss (trace) > > ftrace_cpu_overrun_end[0] 0 info (trace) > > ftrace_cpu_overrun_end[1] 0 info (trace) > > ftrace_cpu_overrun_end[2] 0 info (trace) > > ftrace_cpu_overrun_end[3] 0 info (trace) > > ftrace_cpu_overrun_end[4] 0 info (trace) > > ftrace_cpu_overrun_end[5] 0 info (trace) > > ftrace_cpu_overrun_end[6] 0 info (trace) > > ftrace_cpu_overrun_end[7] 0 info (trace) > > ftrace_cpu_read_events_begin[0] 0 info (trace) > > ftrace_cpu_read_events_begin[1] 0 info (trace) > > ftrace_cpu_read_events_begin[2] 0 info (trace) > > ftrace_cpu_read_events_begin[3] 0 info (trace) > > ftrace_cpu_read_events_begin[4] 0 info (trace) > > ftrace_cpu_read_events_begin[5] 0 info (trace) > > ftrace_cpu_read_events_begin[6] 0 info (trace) > > ftrace_cpu_read_events_begin[7] 0 info (trace) > > ftrace_cpu_read_events_delta[0] 454848 info (trace) > > ftrace_cpu_read_events_delta[1] 453484 info (trace) > > ftrace_cpu_read_events_delta[2] 386290 info (trace) > > ftrace_cpu_read_events_delta[3] 356432 info (trace) > > ftrace_cpu_read_events_delta[4] 393337 info (trace) > > ftrace_cpu_read_events_delta[5] 325244 info (trace) > > ftrace_cpu_read_events_delta[6] 392637 info (trace) > > ftrace_cpu_read_events_delta[7] 350623 info (trace) > > ftrace_cpu_read_events_end[0] 454848 info (trace) > > ftrace_cpu_read_events_end[1] 453484 info (trace) > > ftrace_cpu_read_events_end[2] 386290 info (trace) > > ftrace_cpu_read_events_end[3] 356432 info (trace) > > ftrace_cpu_read_events_end[4] 393337 info (trace) > > ftrace_cpu_read_events_end[5] 325244 info (trace) > > ftrace_cpu_read_events_end[6] 392637 info (trace) > > ftrace_cpu_read_events_end[7] 350623 info (trace) > > ftrace_packet_before_tracing_starthelp_outline 0 info (analysis) > > ftrace_setup_errorshelp_outline 0 error (trace) > > fuchsia_invalid_event 0 error (analysis) > > fuchsia_non_numeric_counters 0 error (analysis) > > fuchsia_timestamp_overflow 0 error (analysis) > > game_intervention_has_parse_errorshelp_outline 0 error (trace) > > game_intervention_has_read_errorshelp_outline 0 error (trace) > > gpu_counters_invalid_spec 0 error (analysis) > > gpu_counters_missing_spec 0 error (analysis) > > gpu_render_stage_parser_errors 0 error (analysis) > > graphics_frame_event_parser_errors 0 info (analysis) > > guess_trace_type_duration_ns 7654 info (analysis) > > heap_graph_non_finalized_graph 0 error (trace) > > heapprofd_missing_packet 0 error (trace) > > heapprofd_non_finalized_profile 0 error (trace) > > interned_data_tokenizer_errors 0 info (analysis) > > invalid_clock_snapshots 0 error (analysis) > > invalid_cpu_times 0 error (analysis) > > json_display_time_unithelp_outline 0 info (trace) > > json_parser_failure 0 error (trace) > > json_tokenizer_failure 0 error (trace) > > meminfo_unknown_keys 0 error (analysis) > > memory_snapshot_parser_failure 0 error (analysis) > > metatrace_overruns 0 error (trace) > > mismatched_sched_switch_tids 11101 error (analysis) > > misplaced_end_event 0 data_loss (analysis) > > mm_unknown_type 0 error (analysis) > > ninja_parse_errors 0 error (trace) > > packages_list_has_parse_errors 0 error (trace) > > packages_list_has_read_errors 0 error (trace) > > parse_trace_duration_ns 1780589548 info (analysis) > > perf_samples_skipped 0 info (trace) > > perf_samples_skipped_dataloss 0 data_loss (trace) > > power_rail_unknown_index 0 error (trace) > > proc_stat_unknown_counters 0 error (analysis) > > process_tracker_errors 0 error (analysis) > > rss_stat_negative_size 0 info (analysis) > > rss_stat_unknown_keys 0 error (analysis) > > rss_stat_unknown_thread_for_mm_id 0 info (analysis) > > sched_switch_out_of_order 0 error (analysis) > > sched_waking_out_of_order 0 error (analysis) > > slice_out_of_order 0 error (analysis) > > sorter_push_event_out_of_orderhelp_outline 0 error (trace) > > stackprofile_invalid_callstack_id 0 error (trace) > > stackprofile_invalid_frame_id 0 error (trace) > > stackprofile_invalid_mapping_id 0 error (trace) > > stackprofile_invalid_string_id 0 error (trace) > > stackprofile_parser_error 0 error (trace) > > symbolization_tmp_build_id_not_foundhelp_outline 0 error (analysis) > > systrace_parse_failure 19040 error (analysis) > > task_state_invalid 0 error (analysis) > > thread_time_in_state_out_of_order 0 error (analysis) > > thread_time_in_state_unknown_cpu_freq 0 error (analysis) > > tokenizer_skipped_packets 0 info (analysis) > > traced_buf_abi_violations[0] 0 data_loss (trace) > > traced_buf_abi_violations[1] 0 data_loss (trace) > > traced_buf_buffer_size[0] 534773760 info (trace) > > traced_buf_buffer_size[1] 2097152 info (trace) > > traced_buf_bytes_overwritten[0] 0 info (trace) > > traced_buf_bytes_overwritten[1] 0 info (trace) > > traced_buf_bytes_read[0] 78929920 info (trace) > > traced_buf_bytes_read[1] 425984 info (trace) > > traced_buf_bytes_written[0] 78962688 info (trace) > > traced_buf_bytes_written[1] 425984 info (trace) > > traced_buf_chunks_committed_out_of_order[0] 0 info (trace) > > traced_buf_chunks_committed_out_of_order[1] 0 info (trace) > > traced_buf_chunks_discarded[0] 0 info (trace) > > traced_buf_chunks_discarded[1] 0 info (trace) > > traced_buf_chunks_overwritten[0] 0 info (trace) > > traced_buf_chunks_overwritten[1] 0 info (trace) > > traced_buf_chunks_read[0] 2428 info (trace) > > traced_buf_chunks_read[1] 13 info (trace) > > traced_buf_chunks_rewritten[0] 6 info (trace) > > traced_buf_chunks_rewritten[1] 0 info (trace) > > traced_buf_chunks_written[0] 2429 info (trace) > > traced_buf_chunks_written[1] 13 info (trace) > > traced_buf_padding_bytes_cleared[0] 0 info (trace) > > traced_buf_padding_bytes_cleared[1] 0 info (trace) > > traced_buf_padding_bytes_written[0] 0 info (trace) > > traced_buf_padding_bytes_written[1] 0 info (trace) > > traced_buf_patches_failed[0] 0 data_loss (trace) > > traced_buf_patches_failed[1] 0 data_loss (trace) > > traced_buf_patches_succeeded[0] 5633 info (trace) > > traced_buf_patches_succeeded[1] 8 info (trace) > > traced_buf_readaheads_failed[0] 115 info (trace) > > traced_buf_readaheads_failed[1] 18 info (trace) > > traced_buf_readaheads_succeeded[0] 2257 info (trace) > > traced_buf_readaheads_succeeded[1] 6 info (trace) > > traced_buf_trace_writer_packet_loss[0] 0 data_loss (trace) > > traced_buf_trace_writer_packet_loss[1] 0 data_loss (trace) > > traced_buf_write_wrap_count[0] 0 info (trace) > > traced_buf_write_wrap_count[1] 0 info (trace) > > traced_chunks_discarded 0 info (trace) > > traced_data_sources_registered 16 info (trace) > > traced_data_sources_seen 6 info (trace) > > traced_final_flush_failed 0 data_loss (trace) > > traced_final_flush_succeeded 0 info (trace) > > traced_flushes_failed 0 data_loss (trace) > > traced_flushes_requested 0 info (trace) > > traced_flushes_succeeded 0 info (trace) > > traced_patches_discarded 0 info (trace) > > traced_producers_connected 3 info (trace) > > traced_producers_seen 3 info (trace) > > traced_total_buffers 2 info (trace) > > traced_tracing_sessions 1 info (trace) > > track_event_dropped_packets_outside_of_range_of_interesthelp_outline 0 info (analysis) > > track_event_parser_errors 0 info (analysis) > > track_event_thread_invalid_endhelp_outline 0 error (trace) > > track_event_tokenizer_errors 0 info (analysis) > > truncated_sys_write_durationhelp_outline 0 data_loss (analysis) > > unknown_extension_fieldshelp_outline 0 error (trace) > > vmstat_unknown_keys 0 error (analysis) > > vulkan_allocations_invalid_string_id 0 error (trace) > > > > > -- > > > Regards > > > Yafang > > > > > > > -- > Regards > Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 15:37 ` Yafang Shao 2023-02-10 18:09 ` Kajetan Puchalski @ 2023-02-11 16:51 ` Qais Yousef 2023-02-12 3:19 ` Yafang Shao 1 sibling, 1 reply; 43+ messages in thread From: Qais Yousef @ 2023-02-11 16:51 UTC (permalink / raw) To: Yafang Shao Cc: Kajetan Puchalski, Mathieu Desnoyers, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Lukasz Luba, Qais Yousef, Daniele Di Proietto On 02/09/23 23:37, Yafang Shao wrote: > On Thu, Feb 9, 2023 at 10:28 PM Kajetan Puchalski > <kajetan.puchalski@arm.com> wrote: > > > > On Thu, Feb 09, 2023 at 02:20:36PM +0800, Yafang Shao wrote: > > > > [...] > > > > Hi Yafang, > > > > > Many thanks for the detailed analysis. Seems it can work. > > > > > > Hi John, > > > > > > Could you pls. try the attached fix ? I have verified it in my test env. > > > > I tested the patch on my environment where I found the issue with newer > > kernels + older Perfetto. The patch does improve things so that's nice. > > Thanks for the test. I don't have Perfetto in hand, so I haven't > verify Perfetto. FWIW, perfetto is not android specific and can run on normal linux distro setup (which I do but haven't noticed this breakage). It's easy to download the latest release (including for android though I never tried that) from github https://github.com/google/perfetto/releases Kajetan might try to see if he can pick the latest version which IIUC contains a workaround. If this simple patch can be tweaked to make it work again against older versions that'd be nice though. HTH. Cheers -- Qais Yousef ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-11 16:51 ` Qais Yousef @ 2023-02-12 3:19 ` Yafang Shao 0 siblings, 0 replies; 43+ messages in thread From: Yafang Shao @ 2023-02-12 3:19 UTC (permalink / raw) To: Qais Yousef Cc: Kajetan Puchalski, Mathieu Desnoyers, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Steven Rostedt, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Lukasz Luba, Qais Yousef, Daniele Di Proietto On Sun, Feb 12, 2023 at 12:51 AM Qais Yousef <qyousef@layalina.io> wrote: > > On 02/09/23 23:37, Yafang Shao wrote: > > On Thu, Feb 9, 2023 at 10:28 PM Kajetan Puchalski > > <kajetan.puchalski@arm.com> wrote: > > > > > > On Thu, Feb 09, 2023 at 02:20:36PM +0800, Yafang Shao wrote: > > > > > > [...] > > > > > > Hi Yafang, > > > > > > > Many thanks for the detailed analysis. Seems it can work. > > > > > > > > Hi John, > > > > > > > > Could you pls. try the attached fix ? I have verified it in my test env. > > > > > > I tested the patch on my environment where I found the issue with newer > > > kernels + older Perfetto. The patch does improve things so that's nice. > > > > Thanks for the test. I don't have Perfetto in hand, so I haven't > > verify Perfetto. > > FWIW, perfetto is not android specific and can run on normal linux distro setup > (which I do but haven't noticed this breakage). > > It's easy to download the latest release (including for android though I never > tried that) from github > > https://github.com/google/perfetto/releases > Thanks for the information. I will try to run it on my test env. I suspect the "systrace_parse_failure" error is caused by the field we introduced into struct ftrace_event_field in the proposed patch, but I haven't taken a deep look at the perfetto src code yet. > Kajetan might try to see if he can pick the latest version which IIUC contains > a workaround. > > If this simple patch can be tweaked to make it work again against older > versions that'd be nice though. > > HTH. > > > Cheers > > -- > Qais Yousef -- Regards Yafang ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 0:54 ` John Stultz 2023-02-09 2:06 ` Mathieu Desnoyers @ 2023-02-09 2:28 ` Steven Rostedt 2023-02-09 2:33 ` Steven Rostedt 1 sibling, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2023-02-09 2:28 UTC (permalink / raw) To: John Stultz Cc: Alexei Starovoitov, Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds On Wed, 8 Feb 2023 16:54:03 -0800 John Stultz <jstultz@google.com> wrote: > > Let me understand what you're saying... > > > > The commit 3087c61ed2c4 did > > > > -/* Task command name length: */ > > -#define TASK_COMM_LEN 16 > > +/* > > + * Define the task command name length as enum, then it can be visible to > > + * BPF programs. > > + */ > > +enum { > > + TASK_COMM_LEN = 16, > > +}; > > > > > > and that caused: > > > > cat /sys/kernel/debug/tracing/events/task/task_newtask/format > > > > to print > > field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; Yes because there's no easy way to automatically convert an enum to a number. And this has been a macro for a very long time (so it works, because macros convert to numbers). And this breaks much more than android. It will break trace-cmd, rasdaemon and perf (if it's not using BTF). This change very much "Breaks userspace!" And requires a kernel workaround, not a user space one. > > instead of > > field:char comm[16]; offset:12; size:16; signed:0; > > > > so the ftrace parsing android tracing tool had to do: > > > > - if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) { > > + if (Match(type_and_name.c_str(), > > + R"(char [a-zA-Z_][a-zA-Z_0-9]*\[[a-zA-Z_0-9]+\])")) { > > > > to workaround this change. > > Right? > > I believe so. > > > And what are you proposing? > > I'm not proposing anything. I was just wanting to understand more > context around this, as it outwardly appears to be a user-breaking > change, and that is usually not done, so I figured it was an issue > worth raising. > > If the debug/tracing/*/format output is in the murky not-really-abi > space, that's fine, but I wanted to know if this was understood as > something that may require userland updates or if this was a > unexpected side-effect. Linus has already said that /sys/kernel/tracing/* is an ABI (fyi, getting to the tracing directory via debugfs is obsolete). Usually, when a trace event uses an enum, it can do: TRACE_DEFINE_ENUM(TASK_COMM_LEN); But that's a very common define. It would require it updated for every trace event, or the change needs to be reverted. Not sure why BTF needs it like this, because it hasn't changed in years. Can't it just hard code it? For ftrace to change it, it requires reading the format files at boot up and replacing the enums with the numbers, which does impact start up. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 2:28 ` Steven Rostedt @ 2023-02-09 2:33 ` Steven Rostedt 2023-02-11 19:00 ` Steven Rostedt 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2023-02-09 2:33 UTC (permalink / raw) To: John Stultz Cc: Alexei Starovoitov, Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds On Wed, 8 Feb 2023 21:28:58 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > And this breaks much more than android. It will break trace-cmd, rasdaemon > and perf (if it's not using BTF). This change very much "Breaks userspace!" > And requires a kernel workaround, not a user space one. OK, so it doesn't break perf, trace-cmd and rasdaemon, because the enum is only needed in the print_fmt part. It can handle it in the field portion. That is: system: sched name: sched_switch ID: 285 format: field:unsigned short common_type; offset:0; size:2; signed:0; field:unsigned char common_flags; offset:2; size:1; signed:0; field:unsigned char common_preempt_count; offset:3; size:1; signed:0; field:int common_pid; offset:4; size:4; signed:1; field:char prev_comm[TASK_COMM_LEN]; offset:8; size:16; signed:0; ^^^^^^^^^^^^^^ ^^ is ignored is used field:pid_t prev_pid; offset:24; size:4; signed:1; field:int prev_prio; offset:28; size:4; signed:1; field:long prev_state; offset:32; size:8; signed:1; field:char next_comm[TASK_COMM_LEN]; offset:40; size:16; signed:0; field:pid_t next_pid; offset:56; size:4; signed:1; field:int next_prio; offset:60; size:4; signed:1; print fmt: "prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d", REC->prev_comm, REC->prev_pid, REC->prev_prio, (REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1)) ? __print_flags(REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1), "|", { 0x00000001, "S" }, { 0x00000002, "D" }, { 0x00000004, "T" }, { 0x00000008, "t" }, { 0x00000010, "X" }, { 0x00000020, "Z" }, { 0x00000040, "P" }, { 0x00000080, "I" }) : "R", REC->prev_state & (((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) ? "+" : "", REC->next_comm, REC->next_pid, REC->next_prio ^^^^^^^ Is what requires the conversions. So I take that back. It only breaks perfetto, and that's because it writes its own parser and doesn't use libtraceevent. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-09 2:33 ` Steven Rostedt @ 2023-02-11 19:00 ` Steven Rostedt 2023-02-12 3:38 ` Yafang Shao 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2023-02-11 19:00 UTC (permalink / raw) To: John Stultz Cc: Alexei Starovoitov, Yafang Shao, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds On Wed, 8 Feb 2023 21:33:43 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > OK, so it doesn't break perf, trace-cmd and rasdaemon, because the enum is > only needed in the print_fmt part. It can handle it in the field portion. > > That is: > > > system: sched > name: sched_switch > ID: 285 > format: > field:unsigned short common_type; offset:0; size:2; signed:0; > field:unsigned char common_flags; offset:2; size:1; signed:0; > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > field:int common_pid; offset:4; size:4; signed:1; > > field:char prev_comm[TASK_COMM_LEN]; offset:8; size:16; signed:0; > ^^^^^^^^^^^^^^ ^^ > is ignored is used > > > field:pid_t prev_pid; offset:24; size:4; signed:1; > field:int prev_prio; offset:28; size:4; signed:1; > field:long prev_state; offset:32; size:8; signed:1; > field:char next_comm[TASK_COMM_LEN]; offset:40; size:16; signed:0; > field:pid_t next_pid; offset:56; size:4; signed:1; > field:int next_prio; offset:60; size:4; signed:1; > > print fmt: "prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d", REC->prev_comm, REC->prev_pid, REC->prev_prio, (REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1)) ? __print_flags(REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1), "|", { 0x00000001, "S" }, { 0x00000002, "D" }, { 0x00000004, "T" }, { 0x00000008, "t" }, { 0x00000010, "X" }, { 0x00000020, "Z" }, { 0x00000040, "P" }, { 0x00000080, "I" }) : "R", REC->prev_state & (((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) ? "+" : "", REC->next_comm, REC->next_pid, REC->next_prio > > ^^^^^^^ > > Is what requires the conversions. So I take that back. It only breaks > perfetto, and that's because it writes its own parser and doesn't use > libtraceevent. Actually, there are cases that this needs to be a number, as b3bc8547d3be6 ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well") made it update fields as well as the printk fmt. I think because libtraceevent noticed that it was a "char" array, it just defaults to "size". But this does have meaning for all other types, and I can see other parsers requiring that. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-11 19:00 ` Steven Rostedt @ 2023-02-12 3:38 ` Yafang Shao 2023-02-12 3:44 ` Steven Rostedt 0 siblings, 1 reply; 43+ messages in thread From: Yafang Shao @ 2023-02-12 3:38 UTC (permalink / raw) To: Steven Rostedt Cc: John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds [-- Attachment #1: Type: text/plain, Size: 3049 bytes --] On Sun, Feb 12, 2023 at 3:00 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 8 Feb 2023 21:33:43 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > OK, so it doesn't break perf, trace-cmd and rasdaemon, because the enum is > > only needed in the print_fmt part. It can handle it in the field portion. > > > > That is: > > > > > > system: sched > > name: sched_switch > > ID: 285 > > format: > > field:unsigned short common_type; offset:0; size:2; signed:0; > > field:unsigned char common_flags; offset:2; size:1; signed:0; > > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > > field:int common_pid; offset:4; size:4; signed:1; > > > > field:char prev_comm[TASK_COMM_LEN]; offset:8; size:16; signed:0; > > ^^^^^^^^^^^^^^ ^^ > > is ignored is used > > > > > > field:pid_t prev_pid; offset:24; size:4; signed:1; > > field:int prev_prio; offset:28; size:4; signed:1; > > field:long prev_state; offset:32; size:8; signed:1; > > field:char next_comm[TASK_COMM_LEN]; offset:40; size:16; signed:0; > > field:pid_t next_pid; offset:56; size:4; signed:1; > > field:int next_prio; offset:60; size:4; signed:1; > > > > print fmt: "prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d", REC->prev_comm, REC->prev_pid, REC->prev_prio, (REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1)) ? __print_flags(REC->prev_state & ((((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) - 1), "|", { 0x00000001, "S" }, { 0x00000002, "D" }, { 0x00000004, "T" }, { 0x00000008, "t" }, { 0x00000010, "X" }, { 0x00000020, "Z" }, { 0x00000040, "P" }, { 0x00000080, "I" }) : "R", REC->prev_state & (((0x00000000 | 0x00000001 | 0x00000002 | 0x00000004 | 0x00000008 | 0x00000010 | 0x00000020 | 0x00000040) + 1) << 1) ? "+" : "", REC->next_comm, REC->next_pid, REC->next_prio > > > > ^^^^^^^ > > > > Is what requires the conversions. So I take that back. It only breaks > > perfetto, and that's because it writes its own parser and doesn't use > > libtraceevent. > > Actually, there are cases that this needs to be a number, as b3bc8547d3be6 > ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well") made > it update fields as well as the printk fmt. > It seems that TRACE_DEFINE_ENUM(TASK_COMM_LEN) in the trace events header files would be a better fix. > I think because libtraceevent noticed that it was a "char" array, it just > defaults to "size". But this does have meaning for all other types, and I > can see other parsers requiring that. > > -- Steve -- Regards Yafang [-- Attachment #2: TASK_COMM_LEN.diff --] [-- Type: application/octet-stream, Size: 2301 bytes --] diff --git a/include/trace/events/block.h b/include/trace/events/block.h index 7f4dfbd..97cf6c2 100644 --- a/include/trace/events/block.h +++ b/include/trace/events/block.h @@ -12,6 +12,8 @@ #define RWBS_LEN 8 +TRACE_DEFINE_ENUM(TASK_COMM_LEN); + DECLARE_EVENT_CLASS(block_buffer, TP_PROTO(struct buffer_head *bh), diff --git a/include/trace/events/oom.h b/include/trace/events/oom.h index 26a11e4..19de9a8 100644 --- a/include/trace/events/oom.h +++ b/include/trace/events/oom.h @@ -7,6 +7,8 @@ #include <linux/tracepoint.h> #include <trace/events/mmflags.h> +TRACE_DEFINE_ENUM(TASK_COMM_LEN); + TRACE_EVENT(oom_score_adj_update, TP_PROTO(struct task_struct *task), diff --git a/include/trace/events/osnoise.h b/include/trace/events/osnoise.h index 82f741e..ac3c0ab 100644 --- a/include/trace/events/osnoise.h +++ b/include/trace/events/osnoise.h @@ -6,6 +6,8 @@ #define _OSNOISE_TRACE_H #include <linux/tracepoint.h> +TRACE_DEFINE_ENUM(TASK_COMM_LEN); + TRACE_EVENT(thread_noise, TP_PROTO(struct task_struct *t, u64 start, u64 duration), diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h index fbb99a6..57ec09e 100644 --- a/include/trace/events/sched.h +++ b/include/trace/events/sched.h @@ -216,6 +216,7 @@ static inline long __trace_sched_switch_state(bool preempt, } #endif /* CREATE_TRACE_POINTS */ +TRACE_DEFINE_ENUM(TASK_COMM_LEN); /* * Tracepoint for task switches, performed by the scheduler: */ diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h index 1db7e4b..3b1cdb6 100644 --- a/include/trace/events/signal.h +++ b/include/trace/events/signal.h @@ -9,6 +9,8 @@ #include <linux/sched.h> #include <linux/tracepoint.h> +TRACE_DEFINE_ENUM(TASK_COMM_LEN); + #define TP_STORE_SIGINFO(__entry, info) \ do { \ if (info == SEND_SIG_NOINFO) { \ diff --git a/include/trace/events/task.h b/include/trace/events/task.h index 64d1609..0a9e03a7 100644 --- a/include/trace/events/task.h +++ b/include/trace/events/task.h @@ -6,6 +6,7 @@ #define _TRACE_TASK_H #include <linux/tracepoint.h> +TRACE_DEFINE_ENUM(TASK_COMM_LEN); TRACE_EVENT(task_newtask, TP_PROTO(struct task_struct *task, unsigned long clone_flags), ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-12 3:38 ` Yafang Shao @ 2023-02-12 3:44 ` Steven Rostedt 2023-02-13 17:43 ` Namhyung Kim 0 siblings, 1 reply; 43+ messages in thread From: Steven Rostedt @ 2023-02-12 3:44 UTC (permalink / raw) To: Yafang Shao Cc: John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds On Sun, 12 Feb 2023 11:38:52 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > Actually, there are cases that this needs to be a number, as b3bc8547d3be6 > > ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well") made > > it update fields as well as the printk fmt. > > > > It seems that TRACE_DEFINE_ENUM(TASK_COMM_LEN) in the trace events > header files would be a better fix. NACK! I much prefer the proper fix that adds the length. -- Steve ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-12 3:44 ` Steven Rostedt @ 2023-02-13 17:43 ` Namhyung Kim 2023-02-13 17:46 ` Mathieu Desnoyers 0 siblings, 1 reply; 43+ messages in thread From: Namhyung Kim @ 2023-02-13 17:43 UTC (permalink / raw) To: Steven Rostedt Cc: Yafang Shao, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Mathieu Desnoyers, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds Hi Steve, On Sat, Feb 11, 2023 at 8:07 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Sun, 12 Feb 2023 11:38:52 +0800 > Yafang Shao <laoar.shao@gmail.com> wrote: > > > > Actually, there are cases that this needs to be a number, as b3bc8547d3be6 > > > ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well") made > > > it update fields as well as the printk fmt. > > > > > > > It seems that TRACE_DEFINE_ENUM(TASK_COMM_LEN) in the trace events > > header files would be a better fix. > > NACK! I much prefer the proper fix that adds the length. Can we just have both enum and macro at the same time? I guess the enum would fill the BTF and the macro would provide backward compatibility. Thanks, Namhyung ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN 2023-02-13 17:43 ` Namhyung Kim @ 2023-02-13 17:46 ` Mathieu Desnoyers 0 siblings, 0 replies; 43+ messages in thread From: Mathieu Desnoyers @ 2023-02-13 17:46 UTC (permalink / raw) To: Namhyung Kim, Steven Rostedt Cc: Yafang Shao, John Stultz, Alexei Starovoitov, Andrew Morton, Network Development, bpf, linux-perf-use., Linux-Fsdevel, linux-mm, LKML, kernel test robot, kbuild test robot, Andrii Nakryiko, David Hildenbrand, Arnaldo Carvalho de Melo, Andrii Nakryiko, Michal Miroslaw, Peter Zijlstra, Matthew Wilcox, Al Viro, Kees Cook, Petr Mladek, Kajetan Puchalski, Lukasz Luba, Qais Yousef, Daniele Di Proietto, Linus Torvalds On 2023-02-13 12:43, Namhyung Kim wrote: > Hi Steve, > > On Sat, Feb 11, 2023 at 8:07 PM Steven Rostedt <rostedt@goodmis.org> wrote: >> >> On Sun, 12 Feb 2023 11:38:52 +0800 >> Yafang Shao <laoar.shao@gmail.com> wrote: >> >>>> Actually, there are cases that this needs to be a number, as b3bc8547d3be6 >>>> ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well") made >>>> it update fields as well as the printk fmt. >>>> >>> >>> It seems that TRACE_DEFINE_ENUM(TASK_COMM_LEN) in the trace events >>> header files would be a better fix. >> >> NACK! I much prefer the proper fix that adds the length. > > Can we just have both enum and macro at the same time? > I guess the enum would fill the BTF and the macro would provide > backward compatibility. This is no need to keep the define. The root cause of the issue is addressed by this fix: https://lore.kernel.org/lkml/20230212154620.4d8fe033@gandalf.local.home/ Thanks, Mathieu > > Thanks, > Namhyung -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2023-02-13 17:46 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-20 11:27 [PATCH v2 0/7] task comm cleanups Yafang Shao 2021-11-20 11:27 ` [PATCH v2 1/7] fs/exec: replace strlcpy with strscpy_pad in __set_task_comm Yafang Shao 2021-11-20 11:27 ` [PATCH v2 2/7] fs/exec: replace strncpy with strscpy_pad in __get_task_comm Yafang Shao 2021-11-20 11:27 ` [PATCH v2 3/7] drivers/infiniband: replace open-coded string copy with get_task_comm Yafang Shao 2021-11-20 11:27 ` [PATCH v2 4/7] fs/binfmt_elf: " Yafang Shao 2021-11-29 16:01 ` Steven Rostedt 2021-11-30 3:01 ` Yafang Shao 2021-11-30 14:22 ` Steven Rostedt 2021-11-30 15:53 ` Yafang Shao 2021-11-20 11:27 ` [PATCH v2 5/7] samples/bpf/test_overhead_kprobe_kern: replace bpf_probe_read_kernel with bpf_probe_read_kernel_str to get task comm Yafang Shao 2021-11-20 11:27 ` [PATCH v2 6/7] tools/bpf/bpftool/skeleton: " Yafang Shao 2021-11-20 11:27 ` [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN Yafang Shao 2021-11-29 10:13 ` Sven Schnelle 2021-11-29 13:41 ` Yafang Shao 2021-11-29 14:21 ` Sven Schnelle 2021-11-29 14:32 ` David Hildenbrand 2021-11-29 14:38 ` Sven Schnelle 2021-11-29 15:33 ` Yafang Shao 2021-11-29 16:07 ` Steven Rostedt 2021-11-29 16:08 ` Steven Rostedt 2021-11-29 15:28 ` Yafang Shao 2021-11-29 17:30 ` Steven Rostedt 2021-11-29 17:56 ` Sven Schnelle 2021-11-30 3:03 ` Yafang Shao 2021-11-30 14:23 ` Steven Rostedt 2021-11-30 15:46 ` Yafang Shao 2023-02-08 21:55 ` John Stultz 2023-02-09 0:10 ` Alexei Starovoitov 2023-02-09 0:54 ` John Stultz 2023-02-09 2:06 ` Mathieu Desnoyers 2023-02-09 6:20 ` Yafang Shao 2023-02-09 14:27 ` Kajetan Puchalski 2023-02-09 15:37 ` Yafang Shao 2023-02-10 18:09 ` Kajetan Puchalski 2023-02-11 16:51 ` Qais Yousef 2023-02-12 3:19 ` Yafang Shao 2023-02-09 2:28 ` Steven Rostedt 2023-02-09 2:33 ` Steven Rostedt 2023-02-11 19:00 ` Steven Rostedt 2023-02-12 3:38 ` Yafang Shao 2023-02-12 3:44 ` Steven Rostedt 2023-02-13 17:43 ` Namhyung Kim 2023-02-13 17:46 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).