Received-SPF: pass (google.com: domain of linux-kernel+bounces-155857-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 23 Apr 2024 17:37:38 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Changbin Du <changbin.du@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] perf trace beauty: Always show param if show_zero
 is set
Message-ID: <ZigcEg1inS1JoSr5@x1>
References: <20240423015330.2206892-1-changbin.du@huawei.com>
 <20240423015330.2206892-2-changbin.du@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240423015330.2206892-2-changbin.du@huawei.com>

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