[PATCH v3 0/8] perf trace: Augment enum arguments with BTF

Howard Chu posted 8 patches 1 year, 7 months ago
There is a newer version of this series
tools/perf/builtin-trace.c               | 229 ++++++++++++++++++++---
tools/perf/tests/builtin-test.c          |   1 +
tools/perf/tests/shell/trace_btf_enum.sh |  61 ++++++
tools/perf/tests/tests.h                 |   1 +
tools/perf/tests/workloads/Build         |   1 +
tools/perf/tests/workloads/landlock.c    |  39 ++++
tools/perf/trace/beauty/beauty.h         |   1 +
tools/perf/util/syscalltbl.c             |   7 +
tools/perf/util/syscalltbl.h             |   1 +
9 files changed, 317 insertions(+), 24 deletions(-)
create mode 100755 tools/perf/tests/shell/trace_btf_enum.sh
create mode 100644 tools/perf/tests/workloads/landlock.c
[PATCH v3 0/8] perf trace: Augment enum arguments with BTF
Posted by Howard Chu 1 year, 7 months ago
In this patch, BTF is used to turn enum value to the corresponding
enum variable name. There is only one system call that uses enum value
as its argument, that is `landlock_add_rule()`.

Enum arguments of non-syscall tracepoints can also be augmented, for
instance timer:hrtimer_start and timer:hrtimer_init's 'mode' argument.

Changes in v3:

- Add trace__btf_scnprintf() helper function
- Remove is_enum memeber in struct syscall_arg_fmt, replace it with 
btf_is_enum()
- Add syscall_arg_fmt__cache_btf_enum() to cache btf_type only
- Resolve NO_LIBBPF=1 build error
- Skip BTF augmentation test if landlock_add_rule syscall and LIBBPF are not
available
- Rename landlock.c workload, add a comment to landlock.c workload
- Change the way of skipping 'enum ' prefix
- Add type_name member to struct syscall_arg

Changes in v2:

- Add trace_btf_enum regression test, and landlock workload

Arnaldo Carvalho de Melo (2):
  perf trace: Introduce trace__btf_scnprintf()
  perf trace: Remove arg_fmt->is_enum, we can get that from the BTF type

Howard Chu (6):
  perf trace: Fix iteration of syscall ids in syscalltbl->entries
  perf trace: BTF-based enum pretty printing for syscall args
  perf trace: Augment non-syscall tracepoints with enum arguments with
    BTF
  perf trace: Filter enum arguments with enum names
  perf test: Add landlock workload
  perf test trace_btf_enum: Add regression test for the BTF augmentation
    of enums in 'perf trace'

 tools/perf/builtin-trace.c               | 229 ++++++++++++++++++++---
 tools/perf/tests/builtin-test.c          |   1 +
 tools/perf/tests/shell/trace_btf_enum.sh |  61 ++++++
 tools/perf/tests/tests.h                 |   1 +
 tools/perf/tests/workloads/Build         |   1 +
 tools/perf/tests/workloads/landlock.c    |  39 ++++
 tools/perf/trace/beauty/beauty.h         |   1 +
 tools/perf/util/syscalltbl.c             |   7 +
 tools/perf/util/syscalltbl.h             |   1 +
 9 files changed, 317 insertions(+), 24 deletions(-)
 create mode 100755 tools/perf/tests/shell/trace_btf_enum.sh
 create mode 100644 tools/perf/tests/workloads/landlock.c

-- 
2.45.2
Re: [PATCH v3 0/8] perf trace: Augment enum arguments with BTF
Posted by Arnaldo Carvalho de Melo 1 year, 7 months ago
On Tue, Jun 25, 2024 at 02:13:37AM +0800, Howard Chu wrote:
> In this patch, BTF is used to turn enum value to the corresponding
> enum variable name. There is only one system call that uses enum value
> as its argument, that is `landlock_add_rule()`.
> 
> Enum arguments of non-syscall tracepoints can also be augmented, for
> instance timer:hrtimer_start and timer:hrtimer_init's 'mode' argument.
> 
> Changes in v3:

Did a quick test, from a quick look you did the adjustments we agreed
(if (val == 0 && !trace->show_zeros && !arg->show_zero && arg->strtoul
!= STUL_BTF_TYPE), etc), thanks!

And that is the way for collaboration we go on talking on the mailing
list and sometimes writing code, making it available for review and
adopting what we deem best at that point, rinse repeat.

Now I think it would be great if someone like Namhyung or Ian could try
this last patch.

I have to comb thru it but, again, from a quick look and test, it seems
great and probably ready for merging.

Thanks a lot!

- Arnaldo
 
> - Add trace__btf_scnprintf() helper function
> - Remove is_enum memeber in struct syscall_arg_fmt, replace it with 
> btf_is_enum()
> - Add syscall_arg_fmt__cache_btf_enum() to cache btf_type only
> - Resolve NO_LIBBPF=1 build error
> - Skip BTF augmentation test if landlock_add_rule syscall and LIBBPF are not
> available
> - Rename landlock.c workload, add a comment to landlock.c workload
> - Change the way of skipping 'enum ' prefix
> - Add type_name member to struct syscall_arg
> 
> Changes in v2:
> 
> - Add trace_btf_enum regression test, and landlock workload
> 
> Arnaldo Carvalho de Melo (2):
>   perf trace: Introduce trace__btf_scnprintf()
>   perf trace: Remove arg_fmt->is_enum, we can get that from the BTF type
> 
> Howard Chu (6):
>   perf trace: Fix iteration of syscall ids in syscalltbl->entries
>   perf trace: BTF-based enum pretty printing for syscall args
>   perf trace: Augment non-syscall tracepoints with enum arguments with
>     BTF
>   perf trace: Filter enum arguments with enum names
>   perf test: Add landlock workload
>   perf test trace_btf_enum: Add regression test for the BTF augmentation
>     of enums in 'perf trace'
> 
>  tools/perf/builtin-trace.c               | 229 ++++++++++++++++++++---
>  tools/perf/tests/builtin-test.c          |   1 +
>  tools/perf/tests/shell/trace_btf_enum.sh |  61 ++++++
>  tools/perf/tests/tests.h                 |   1 +
>  tools/perf/tests/workloads/Build         |   1 +
>  tools/perf/tests/workloads/landlock.c    |  39 ++++
>  tools/perf/trace/beauty/beauty.h         |   1 +
>  tools/perf/util/syscalltbl.c             |   7 +
>  tools/perf/util/syscalltbl.h             |   1 +
>  9 files changed, 317 insertions(+), 24 deletions(-)
>  create mode 100755 tools/perf/tests/shell/trace_btf_enum.sh
>  create mode 100644 tools/perf/tests/workloads/landlock.c
> 
> -- 
> 2.45.2
Re: [PATCH v3 0/8] perf trace: Augment enum arguments with BTF
Posted by Namhyung Kim 1 year, 7 months ago
On Mon, Jun 24, 2024 at 07:06:26PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Jun 25, 2024 at 02:13:37AM +0800, Howard Chu wrote:
> > In this patch, BTF is used to turn enum value to the corresponding
> > enum variable name. There is only one system call that uses enum value
> > as its argument, that is `landlock_add_rule()`.
> > 
> > Enum arguments of non-syscall tracepoints can also be augmented, for
> > instance timer:hrtimer_start and timer:hrtimer_init's 'mode' argument.
> > 
> > Changes in v3:
> 
> Did a quick test, from a quick look you did the adjustments we agreed
> (if (val == 0 && !trace->show_zeros && !arg->show_zero && arg->strtoul
> != STUL_BTF_TYPE), etc), thanks!
> 
> And that is the way for collaboration we go on talking on the mailing
> list and sometimes writing code, making it available for review and
> adopting what we deem best at that point, rinse repeat.
> 
> Now I think it would be great if someone like Namhyung or Ian could try
> this last patch.
> 
> I have to comb thru it but, again, from a quick look and test, it seems
> great and probably ready for merging.

So I'm trying to test this but I got a build error like this:

tests/workloads/landlock.c: In function ‘landlock’:
tests/workloads/landlock.c:22:16: error: variable ‘net_port_attr’ has initializer but incomplete type
   22 |         struct landlock_net_port_attr net_port_attr = {
      |                ^~~~~~~~~~~~~~~~~~~~~~
tests/workloads/landlock.c:23:18: error: ‘struct landlock_net_port_attr’ has no member named ‘port’
   23 |                 .port = 19,
      |                  ^~~~
tests/workloads/landlock.c:23:25: error: excess elements in struct initializer [-Werror]
   23 |                 .port = 19,
      |                         ^~
tests/workloads/landlock.c:23:25: note: (near initialization for ‘net_port_attr’)
tests/workloads/landlock.c:24:18: error: ‘struct landlock_net_port_attr’ has no member named ‘allowed_access’
   24 |                 .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
      |                  ^~~~~~~~~~~~~~
tests/workloads/landlock.c:24:35: error: ‘LANDLOCK_ACCESS_NET_CONNECT_TCP’ undeclared (first use in this function); did you mean ‘LANDLOCK_ACCESS_FS_TRUNCATE’?
   24 |                 .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                   LANDLOCK_ACCESS_FS_TRUNCATE
tests/workloads/landlock.c:24:35: note: each undeclared identifier is reported only once for each function it appears in
tests/workloads/landlock.c:24:35: error: excess elements in struct initializer [-Werror]
tests/workloads/landlock.c:24:35: note: (near initialization for ‘net_port_attr’)
tests/workloads/landlock.c:22:39: error: storage size of ‘net_port_attr’ isn’t known
   22 |         struct landlock_net_port_attr net_port_attr = {
      |                                       ^~~~~~~~~~~~~
tests/workloads/landlock.c:30:45: error: ‘LANDLOCK_RULE_NET_PORT’ undeclared (first use in this function); did you mean ‘LANDLOCK_RULE_PATH_BENEATH’?
   30 |         syscall(__NR_landlock_add_rule, fd, LANDLOCK_RULE_NET_PORT,
      |                                             ^~~~~~~~~~~~~~~~~~~~~~
      |                                             LANDLOCK_RULE_PATH_BENEATH
tests/workloads/landlock.c:22:39: error: unused variable ‘net_port_attr’ [-Werror=unused-variable]
   22 |         struct landlock_net_port_attr net_port_attr = {
      |                                       ^~~~~~~~~~~~~

And I found it's reported before:
https://lore.kernel.org/linux-perf-users/Zn5aGnyjyCqAX+66@rli9-mobl/

It seems my system has an old copy of the linux/landlock.h and it
doesn't have the new struct and macro.  I think you can change it to
include what we have in the source tree.  Maybe like

#include "../../../../include/uapi/linux/landlock.h"

Probably the same for the syscall number by reading it from unistd.h in
the source tree.  Then we probably don't need to make it conditional as
we use the latest version.  But I'm not sure if it's ok to read the
syscall number from asm-generic header since it might be different on
other arch.

Thanks,
Namhyung