Re: [PATCH v2 1/2] perf trace beauty: Always show param if show_zero is set

[duchangbin]

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 <changbin.du@xxxxxxxxxx>
> > ---
> >  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