Date: Thu, 13 Jun 2024 23:50:59 +0800
From: Howard Chu <howardchu95@...il.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: peterz@...radead.org, mingo@...hat.com, namhyung@...nel.org, 
	mark.rutland@....com, alexander.shishkin@...ux.intel.com, jolsa@...nel.org, 
	irogers@...gle.com, adrian.hunter@...el.com, kan.liang@...ux.intel.com, 
	mic@...ikod.net, gnoack@...gle.com, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [PATCH v4] perf trace: BTF-based enum pretty printing

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@...nel.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@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@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@x1:~#
>
> Cool, it works!
>
> Now lets try and use it with filters, to get something other than HRTIMER_MODE_ABS:
>
> root@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@x1:~#
>
>
> oops, that is the next step then :-)

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

>
> If I do:
>
> root@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@x1:~#
>
> And then use the value for HRTIMER_MODE_ABS instead:
>
> root@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@x1:~#
>
> Now also filtering HRTIMER_MODE_ABS_PINNED_HARD:
>
> root@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@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@...lbox 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

Powered by blists - more mailing lists