v2:
- Remove bugfix since the code is refactored in perf-tools-next.
- Also apply to mmap, pkey_mprotect.
Changbin Du (2):
perf trace beauty: Always show param if show_zero is set
perf trace beauty: Always show mmap prot even though PROT_NONE
tools/perf/builtin-trace.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
--
2.34.1
PROT_NONE is also useful information, so do not omit the mmap prot even
though it is 0. syscall_arg__scnprintf_mmap_prot() could print PROT_NONE
for prot 0.
Signed-off-by: Changbin Du <[email protected]>
---
tools/perf/builtin-trace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index a8407eee58a3..e82ed002890f 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1033,7 +1033,7 @@ static const struct syscall_fmt syscall_fmts[] = {
#if defined(__s390x__)
.alias = "old_mmap",
#endif
- .arg = { [2] = { .scnprintf = SCA_MMAP_PROT, /* prot */ },
+ .arg = { [2] = { .scnprintf = SCA_MMAP_PROT, .show_zero = true, /* prot */ },
[3] = { .scnprintf = SCA_MMAP_FLAGS, /* flags */
.strtoul = STUL_STRARRAY_FLAGS,
.parm = &strarray__mmap_flags, },
@@ -1050,7 +1050,7 @@ static const struct syscall_fmt syscall_fmts[] = {
[4] = { .scnprintf = SCA_MOVE_MOUNT_FLAGS, /* flags */ }, }, },
{ .name = "mprotect",
.arg = { [0] = { .scnprintf = SCA_HEX, /* start */ },
- [2] = { .scnprintf = SCA_MMAP_PROT, /* prot */ }, }, },
+ [2] = { .scnprintf = SCA_MMAP_PROT, .show_zero = true, /* prot */ }, }, },
{ .name = "mq_unlink",
.arg = { [0] = { .scnprintf = SCA_FILENAME, /* u_name */ }, }, },
{ .name = "mremap", .hexret = true,
@@ -1084,7 +1084,7 @@ static const struct syscall_fmt syscall_fmts[] = {
.arg = { [0] = { .scnprintf = SCA_INT, /* key */ }, }, },
{ .name = "pkey_mprotect",
.arg = { [0] = { .scnprintf = SCA_HEX, /* start */ },
- [2] = { .scnprintf = SCA_MMAP_PROT, /* prot */ },
+ [2] = { .scnprintf = SCA_MMAP_PROT, .show_zero = true, /* prot */ },
[3] = { .scnprintf = SCA_INT, /* pkey */ }, }, },
{ .name = "poll", .timeout = true, },
{ .name = "ppoll", .timeout = true, },
--
2.34.1
For some parameters, it is best to also display them when they are 0,
e.g. flags.
Signed-off-by: Changbin Du <[email protected]>
---
tools/perf/builtin-trace.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index e5fef39c34bf..a8407eee58a3 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2099,9 +2099,9 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
!trace->show_zeros &&
!(sc->arg_fmt &&
(sc->arg_fmt[arg.idx].show_zero ||
- sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
- sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
- sc->arg_fmt[arg.idx].parm))
+ ((sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
+ sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
+ sc->arg_fmt[arg.idx].parm))))
continue;
printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
@@ -2803,8 +2803,8 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
*/
if (val == 0 &&
!trace->show_zeros &&
- !((arg->show_zero ||
- arg->scnprintf == SCA_STRARRAY ||
+ !arg->show_zero &&
+ !((arg->scnprintf == SCA_STRARRAY ||
arg->scnprintf == SCA_STRARRAYS) &&
arg->parm))
continue;
--
2.34.1
On Tue, Apr 23, 2024 at 09:53:29AM +0800, Changbin Du wrote:
> For some parameters, it is best to also display them when they are 0,
> e.g. flags.
Please provide examples of what you're changing, to understand your
change one has to know what are strarrays and what they handle in 'perf
trace', i.e. if the value is zero but the argument has a string array
associated, it probably will translate zero into some string.
So I did:
root@x1:~# perf trace -e syscalls:sys_enter_mmap --filter prot==0
0.000 gnome-shell/2293 syscalls:sys_enter_mmap(addr: 0x10afec7e1000, len: 65536, flags: PRIVATE|FIXED|ANONYMOUS)
^Croot@x1:~#
And this is _before_ this patch, after this patch we get:
root@x1:~# perf trace -e syscalls:sys_enter_mmap --filter prot==0
0.000 Isolated Web C/25530 syscalls:sys_enter_mmap(addr: 0x7f27df529000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
40.267 DOM Worker/1105511 syscalls:sys_enter_mmap(addr: 0x1c9faec48000, len: 65536, flags: PRIVATE|FIXED|ANONYMOUS)
270.145 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed343000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
2194.686 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed39f000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
2461.709 Isolated Web C/21794 syscalls:sys_enter_mmap(addr: 0x7fdc3e100000, len: 1048576, flags: PRIVATE|FIXED|ANONYMOUS)
4476.053 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed3a1000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
^Croot@x1:~#
Because 'mmap's 'prot' is not set directly as an strarray, see:
{ .name = "mmap", .hexret = true,
/* The standard mmap maps to old_mmap on s390x */
#if defined(__s390x__)
.alias = "old_mmap",
#endif
.arg = { [2] = { .scnprintf = SCA_MMAP_PROT, /* prot */ },
[3] = { .scnprintf = SCA_MMAP_FLAGS, /* flags */
.strtoul = STUL_STRARRAY_FLAGS,
.parm = &strarray__mmap_flags, },
[5] = { .scnprintf = SCA_HEX, /* offset */ }, }, },
static size_t syscall_arg__scnprintf_mmap_prot(char *bf, size_t size, struct syscall_arg *arg)
{
unsigned long prot = arg->val;
if (prot == 0)
return scnprintf(bf, size, "%sNONE", arg->show_string_prefix ? strarray__mmap_prot.prefix : "");
return mmap__scnprintf_prot(prot, bf, size, arg->show_string_prefix);
}
#define SCA_MMAP_PROT syscall_arg__scnprintf_mmap_prot
So can you please provide an example that shows before/after your patch?
- Arnaldo
> Signed-off-by: Changbin Du <[email protected]>
> ---
> tools/perf/builtin-trace.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index e5fef39c34bf..a8407eee58a3 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2099,9 +2099,9 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> !trace->show_zeros &&
> !(sc->arg_fmt &&
> (sc->arg_fmt[arg.idx].show_zero ||
> - sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
> - sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
> - sc->arg_fmt[arg.idx].parm))
> + ((sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
> + sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
> + sc->arg_fmt[arg.idx].parm))))
> continue;
>
> printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
> @@ -2803,8 +2803,8 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> */
> if (val == 0 &&
> !trace->show_zeros &&
> - !((arg->show_zero ||
> - arg->scnprintf == SCA_STRARRAY ||
> + !arg->show_zero &&
> + !((arg->scnprintf == SCA_STRARRAY ||
> arg->scnprintf == SCA_STRARRAYS) &&
> arg->parm))
> continue;
> --
> 2.34.1
On Tue, Apr 23, 2024 at 05:37:38PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Apr 23, 2024 at 09:53:29AM +0800, Changbin Du wrote:
> > For some parameters, it is best to also display them when they are 0,
> > e.g. flags.
>
> Please provide examples of what you're changing, to understand your
> change one has to know what are strarrays and what they handle in 'perf
> trace', i.e. if the value is zero but the argument has a string array
> associated, it probably will translate zero into some string.
>
How about only check the show_zero property? The field itself knows exactly what it
wants.
if (val == 0 &&
!trace->show_zeros &&
!(sc->arg_fmt && sc->arg_fmt[arg.idx].show_zero))
continue;
> So I did:
>
> root@x1:~# perf trace -e syscalls:sys_enter_mmap --filter prot==0
> 0.000 gnome-shell/2293 syscalls:sys_enter_mmap(addr: 0x10afec7e1000, len: 65536, flags: PRIVATE|FIXED|ANONYMOUS)
> ^Croot@x1:~#
>
> And this is _before_ this patch, after this patch we get:
>
>
> root@x1:~# perf trace -e syscalls:sys_enter_mmap --filter prot==0
> 0.000 Isolated Web C/25530 syscalls:sys_enter_mmap(addr: 0x7f27df529000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
> 40.267 DOM Worker/1105511 syscalls:sys_enter_mmap(addr: 0x1c9faec48000, len: 65536, flags: PRIVATE|FIXED|ANONYMOUS)
> 270.145 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed343000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
> 2194.686 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed39f000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
> 2461.709 Isolated Web C/21794 syscalls:sys_enter_mmap(addr: 0x7fdc3e100000, len: 1048576, flags: PRIVATE|FIXED|ANONYMOUS)
> 4476.053 firefox/3447 syscalls:sys_enter_mmap(addr: 0x7fa0ed3a1000, len: 4096, flags: PRIVATE|FIXED|ANONYMOUS)
> ^Croot@x1:~#
>
It doesn't seem to be effective on your perf. Here is my example.
Before: PROT_NONE is not shown.
$ sudo perf trace -e syscalls:sys_enter_mmap --filter prot==0 -- ls
0.000 ls/2979231 syscalls:sys_enter_mmap(len: 4220888, flags: PRIVATE|ANONYMOUS)
After: PROT_NONE is displayed.
$ sudo perf trace -e syscalls:sys_enter_mmap --filter prot==0 -- ls
0.000 ls/2975708 syscalls:sys_enter_mmap(len: 4220888, prot: NONE, flags: PRIVATE|ANONYMOUS)
> Because 'mmap's 'prot' is not set directly as an strarray, see:
>
> { .name = "mmap", .hexret = true,
> /* The standard mmap maps to old_mmap on s390x */
> #if defined(__s390x__)
> .alias = "old_mmap",
> #endif
> .arg = { [2] = { .scnprintf = SCA_MMAP_PROT, /* prot */ },
> [3] = { .scnprintf = SCA_MMAP_FLAGS, /* flags */
> .strtoul = STUL_STRARRAY_FLAGS,
> .parm = &strarray__mmap_flags, },
> [5] = { .scnprintf = SCA_HEX, /* offset */ }, }, },
>
> static size_t syscall_arg__scnprintf_mmap_prot(char *bf, size_t size, struct syscall_arg *arg)
> {
> unsigned long prot = arg->val;
>
> if (prot == 0)
> return scnprintf(bf, size, "%sNONE", arg->show_string_prefix ? strarray__mmap_prot.prefix : "");
>
> return mmap__scnprintf_prot(prot, bf, size, arg->show_string_prefix);
> }
>
> #define SCA_MMAP_PROT syscall_arg__scnprintf_mmap_prot
>
> So can you please provide an example that shows before/after your patch?
>
> - Arnaldo
>
> > Signed-off-by: Changbin Du <[email protected]>
> > ---
> > tools/perf/builtin-trace.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index e5fef39c34bf..a8407eee58a3 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -2099,9 +2099,9 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> > !trace->show_zeros &&
> > !(sc->arg_fmt &&
> > (sc->arg_fmt[arg.idx].show_zero ||
> > - sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
> > - sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
> > - sc->arg_fmt[arg.idx].parm))
> > + ((sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAY ||
> > + sc->arg_fmt[arg.idx].scnprintf == SCA_STRARRAYS) &&
> > + sc->arg_fmt[arg.idx].parm))))
> > continue;
> >
> > printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
> > @@ -2803,8 +2803,8 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> > */
> > if (val == 0 &&
> > !trace->show_zeros &&
> > - !((arg->show_zero ||
> > - arg->scnprintf == SCA_STRARRAY ||
> > + !arg->show_zero &&
> > + !((arg->scnprintf == SCA_STRARRAY ||
> > arg->scnprintf == SCA_STRARRAYS) &&
> > arg->parm))
> > continue;
> > --
> > 2.34.1
--
Cheers,
Changbin Du