Re: [PATCH v3] perf record: encode -k clockid frequency into Perf trace

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Sat, Oct 06, 2018 at 04:14:11PM +0300, Alexey Budankov escreveu:
> On 06.10.2018 14:51, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Oct 04, 2018 at 02:36:57PM +0200, Jiri Olsa escreveu:
> >> On Wed, Oct 03, 2018 at 07:57:12PM +0300, Alexey Budankov wrote:
> >>>
> >>> Store -k clockid frequency into Perf trace to enable timestamps 
> >>> derived metrics conversion into wall clock time on reporting stage.
> >>>
> >>> Below is the example of perf report output:
> >>>
> >>> tools/perf/perf record -k raw -- ../../matrix/linux/matrix.gcc
> >>> ...
> >>> [ perf record: Captured and wrote 31.222 MB perf.data (818054 samples) ]
> >>>
> >>> tools/perf/perf report --header
> >>> # ========
> >>> ...
> >>> # event : name = cycles:ppp, , size = 112, { sample_period, sample_freq } = 4000, sample_type = IP|TID|TIME|PERIOD, disabled = 1, inherit = 1, mmap = 1, comm = 1, freq = 1, enable_on_exec = 1, task = 1, precise_ip = 3, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1, use_clockid = 1, clockid = 4
> >>> ...
> >>> # clockid frequency: 1000 MHz
> >>> ...
> >>> # ========
> >>>
> >>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >> Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > 
> > 
> >   CC       /tmp/build/perf/builtin-script.o
> >   CC       /tmp/build/perf/util/header.o
> > util/header.c: In function 'print_clockid':
> > util/header.c:1520:38: error: format '%ld' expects argument of type 'long int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Werror=format=]
> >   fprintf(fp, "# clockid frequency: %ld MHz\n",
> >                                     ~~^
> >                                     %d
> >    ff->ph->env.clockid_res_ns * 1000);
> >    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   
> > util/header.c: In function 'process_clockid':
> > util/header.c:2552:22: error: passing argument 2 of 'do_read_u64' from incompatible pointer type [-Werror=incompatible-pointer-types]
> >   if (do_read_u64(ff, &clockid_res_ns))
> >                       ^~~~~~~~~~~~~~~
> > util/header.c:229:49: note: expected 'u64 *' {aka 'long long unsigned int *'} but argument is of type 'size_t *' {aka 'unsigned int *'}
> >  static int do_read_u64(struct feat_fd *ff, u64 *addr)
> >                                             ~~~~~^~~~
> > In file included from /usr/mips-linux-gnu/include/string.h:494,
> >                  from util/string2.h:7,
> >                  from util/header.c:5:
> > In function 'memcpy',
> >     inlined from '__do_read_buf' at util/header.c:202:2,
> >     inlined from '__do_read' at util/header.c:213:9,
> >     inlined from 'do_read_u64' at util/header.c:233:8,
> >     inlined from 'process_clockid' at util/header.c:2552:6:
> > /usr/mips-linux-gnu/include/bits/string_fortified.h:34:10: error: '__builtin___memcpy_chk' forming offset [5, 8] is out of the bounds [0, 4] of object 'clockid_res_ns' with type 'size_t' {aka 'unsigned int'} [-Werror=array-bounds]
> >    return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
> >           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > util/header.c: In function 'process_clockid':
> > util/header.c:2550:9: note: 'clockid_res_ns' declared here
> >   size_t clockid_res_ns;
> >          ^~~~~~~~~~~~~~
> >   MKDIR    /tmp/build/perf/scripts/
> >   LD       /tmp/build/perf/scripts/libperf-in.o
> >   CC       /tmp/build/perf/builtin-kmem.o
> >   CC       /tmp/build/perf/builtin-lock.o
> >   CC       /tmp/build/perf/ui/browsers/map.o
> >   CC       /tmp/build/perf/builtin-kvm.o
> > cc1: all warnings being treated as errors
> > mv: cannot stat '/tmp/build/perf/util/.header.o.tmp': No such file or directory
> > make[4]: *** [/git/linux/tools/build/Makefile.build:97: /tmp/build/perf/util/header.o] Error 1
> > make[3]: *** [/git/linux/tools/build/Makefile.build:139: util] Error 2
> > make[3]: *** Waiting for unfinished jobs....
> > 
> 
> Hope this helps.
> 
> ---
> tools/perf/util/header.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 4ce5339158f7..afaebbb53035 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1517,7 +1517,7 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp)
>  
>  static void print_clockid(struct feat_fd *ff, FILE *fp)
>  {
> -       fprintf(fp, "# clockid frequency: %ld MHz\n",
> +       fprintf(fp, "# clockid frequency: %zd MHz\n",
>                 ff->ph->env.clockid_res_ns * 1000);
>  }
>  
> @@ -2549,7 +2549,7 @@ static int process_clockid(struct feat_fd *ff,
>  {
>         size_t clockid_res_ns;
>  
> -       if (do_read_u64(ff, &clockid_res_ns))
> +       if (__do_read(ff, &clockid_res_ns, sizeof(clockid_res_ns)))
>                 return -1;
>  
>         ff->ph->env.clockid_res_ns = clockid_res_ns;

Are you sure about that? Isn't this always encoded as an u64? Also, look
at what else do_read_u64 does besides just reading:

static int do_read_u64(struct feat_fd *ff, u64 *addr)
{
        int ret;

        ret = __do_read(ff, addr, sizeof(*addr));
        if (ret)
                return ret;

        if (ff->ph->needs_swap)
                *addr = bswap_64(*addr);
        return 0;
}

/me goes to look at the original patch to see how you encode that
clod_id_res_ns in the perf.data header...

- Arnaldo