[PATCH v4] perf trace: BTF-based enum pretty printing

Howard Chu howardchu95 at gmail.com
Thu Jun 13 15:50:59 UTC 2024


Hello Arnaldo,

Thanks for testing and reviewing this patch, and your precious suggestions.

On Thu, Jun 13, 2024 at 8:59 PM Arnaldo Carvalho de Melo
<acme at kernel.org> wrote:
>
> On Thu, Jun 13, 2024 at 09:47:02AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Thu, Jun 13, 2024 at 12:27:47PM +0800, Howard Chu wrote:
> > > changes in v4:
>
> > > - Add enum support to tracepoint arguments
>
> > That is cool, but see below the comment as having this as a separate
> > patch.
> >
> > Also please, on the patch that introduces ! syscall tracepoint enum args
> > BTF augmentation include examples of tracepoints being augmented. I'll
>
> You did it as a notes for v4, great, I missed that.
>
> > try here while testing the patch as-is.
>
> The landlock_add_rule continues to work, using the same test program I
> posted when testing your v1 patch:
>
> root at x1:~# perf trace -e landlock_add_rule
>      0.000 ( 0.016 ms): landlock_add_r/475518 landlock_add_rule(ruleset_fd: 1, rule_type: LANDLOCK_RULE_PATH_BENEATH, rule_attr: 0x7ffd790ff690) = -1 EBADFD (File descriptor in bad state)
>      0.115 ( 0.003 ms): landlock_add_r/475518 landlock_add_rule(ruleset_fd: 2, rule_type: LANDLOCK_RULE_NET_PORT, rule_attr: 0x7ffd790ff690) = -1 EBADFD (File descriptor in bad state)
>
> Now lets try with some of the !syscalls tracepoints with enum args:
>
> root at x1:~# perf trace -e timer:hrtimer_start --max-events=5
>      0.000 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff225050, function: 0xffffffff9e22ddd0, expires: 210588861000000, softexpires: 210588861000000, mode: HRTIMER_MODE_ABS)
> 18446744073709.551 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff2a5050, function: 0xffffffff9e22ddd0, expires: 210588861000000, softexpires: 210588861000000, mode: HRTIMER_MODE_ABS)
>      0.007 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff325050, function: 0xffffffff9e22ddd0, expires: 210588861000000, softexpires: 210588861000000, mode: HRTIMER_MODE_ABS)
>      0.007 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff3a5050, function: 0xffffffff9e22ddd0, expires: 210588861000000, softexpires: 210588861000000, mode: HRTIMER_MODE_ABS)
> 18446744073709.543 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff425050, function: 0xffffffff9e22ddd0, expires: 210588861000000, softexpires: 210588861000000, mode: HRTIMER_MODE_ABS)
> root at x1:~#
>
> Cool, it works!
>
> Now lets try and use it with filters, to get something other than HRTIMER_MODE_ABS:
>
> root at x1:~# perf trace -e timer:hrtimer_start --filter='mode!=HRTIMER_MODE_ABS' --max-events=5
> No resolver (strtoul) for "mode" in "timer:hrtimer_start", can't set filter "(mode!=HRTIMER_MODE_ABS) && (common_pid != 475859 && common_pid != 4041)"
> root at x1:~#
>
>
> oops, that is the next step then :-)

Sure, I will add support for enum filtering(enum string -> int).

>
> If I do:
>
> root at x1:~# pahole --contains_enumerator=HRTIMER_MODE_ABS
> enum hrtimer_mode {
>         HRTIMER_MODE_ABS             = 0,
>         HRTIMER_MODE_REL             = 1,
>         HRTIMER_MODE_PINNED          = 2,
>         HRTIMER_MODE_SOFT            = 4,
>         HRTIMER_MODE_HARD            = 8,
>         HRTIMER_MODE_ABS_PINNED      = 2,
>         HRTIMER_MODE_REL_PINNED      = 3,
>         HRTIMER_MODE_ABS_SOFT        = 4,
>         HRTIMER_MODE_REL_SOFT        = 5,
>         HRTIMER_MODE_ABS_PINNED_SOFT = 6,
>         HRTIMER_MODE_REL_PINNED_SOFT = 7,
>         HRTIMER_MODE_ABS_HARD        = 8,
>         HRTIMER_MODE_REL_HARD        = 9,
>         HRTIMER_MODE_ABS_PINNED_HARD = 10,
>         HRTIMER_MODE_REL_PINNED_HARD = 11,
> }
> root at x1:~#
>
> And then use the value for HRTIMER_MODE_ABS instead:
>
> root at x1:~# perf trace -e timer:hrtimer_start --filter='mode!=0' --max-events=1
>      0.000 :0/0 timer:hrtimer_start(hrtimer: 0xffff8d4eff225050, function: 0xffffffff9e22ddd0, expires: 210759990000000, softexpires: 210759990000000, mode: HRTIMER_MODE_ABS_PINNED_HARD)
> root at x1:~#
>
> Now also filtering HRTIMER_MODE_ABS_PINNED_HARD:
>
> root at x1:~# perf trace -e timer:hrtimer_start --filter='mode!=0 && mode != 10' --max-events=2
>      0.000 podman/178137 timer:hrtimer_start(hrtimer: 0xffffa2024468fda8, function: 0xffffffff9e2170c0, expires: 210886679225214, softexpires: 210886679175214, mode: HRTIMER_MODE_REL)
>     32.935 podman/5046 timer:hrtimer_start(hrtimer: 0xffffa20244fabc40, function: 0xffffffff9e2170c0, expires: 210886712159707, softexpires: 210886712109707, mode: HRTIMER_MODE_REL)
> root at x1:~#
>
> But this then should be a _third_ patch :-)

Sure.

>
> We're making progress!
>
> - Arnaldo

> See the comment about evsel__init_tp_arg_scnprintf() below. Also please
> do patches on top of previous work, i.e. the v3 patch should be a
> separate patch and this v4 should add the extra functionality, i.e. the
> support for !syscall tracepoint enum BTF augmentation.

Thank you for suggesting this. May I ask if this is saying that v3 and
v4 should all be separated?

> The convention here is that evsel__ is the "class" name, so the first
> arg is a 'struct evsel *', if you really were transforming this into a
> 'struct trace' specific "method" you would change the name of the C
> function to 'trace__init_tp_arg_scnprintf'.

Oops, my bad. Thanks for pointing it out.

>
> But in this case instead of passing the 'struct trace' pointer all the
> way down we should instead pass a 'bool *use_btf' argument, making it:
>
>
> static int evsel__init_tp_arg_scnprintf(struct evsel *evsel, bool *use_btf)

You are right, we should do that. Thanks for pointing out this silly
implementation. I think we should do the same for
syscall__set_arg_fmts(struct trace *trace, struct syscall *sc) as
well. Also, I forgot to delete the unused 'bool use_btf' in struct
syscall, I will delete it.

>
> Then, when evlist__set_syscall_tp_fields(evlist, &use_btf) returns,
> check that use_btf to check if we need to call
> trace__load_vmlinux_btf(trace).

> And when someone suggests you do something and you implement it, a
> Suggested-by: tag is as documented in:
>
> ⬢[acme at toolbox perf-tools-next]$ grep -A5 Suggested-by Documentation/process/submitting-patches.rst
> Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:

> A Suggested-by: tag indicates that the patch idea is suggested by the person
> named and ensures credit to the person for the idea. Please note that this
> tag should not be added without the reporter's permission

May I ask if you want a Suggested-by? Hats off to you sir.

Also, do you want me to do the fixes on evsel__init_tp_arg_scnprintf()
for tracepoint enum, and send it as v5, or just send a separate patch
for tracepoint enum, so we get a patch for syscall enum, and another
patch for tracepoint enum? Sorry to bother you on these trivial
things.

Thanks again for this detailed review, and valuable suggestions.

Thanks,
Howard



More information about the Linux-security-module-archive mailing list