linux-kernel - Re: [PATCH] perf/stat: Fix perf stat for forked applications

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Mar 2022 13:54:23 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Thomas Richter <tmricht@...ux.ibm.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        gor@...ux.ibm.com, Arnaldo Carvalho de Melo <acme@...hat.com>,
        Sven Schnelle <svens@...ux.ibm.com>, sumanthk@...ux.ibm.com,
        Heiko Carstens <hca@...ux.ibm.com>
Subject: Re: [PATCH] perf/stat: Fix perf stat for forked applications

On Wed, Mar 23, 2022 at 4:07 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Thu, Mar 17, 2022 at 04:53:46PM +0100, Thomas Richter wrote:
> > I have run into the following issue:
> >
> >  # perf stat -a -e new_pmu/INSTRUCTION_7/ --  mytest -c1 7
> >
> >  Performance counter stats for 'system wide':
> >
> >                  0      new_pmu/INSTRUCTION_7/
> >
> >        0.000366428 seconds time elapsed
> >  #
> >
> > The new PMU for s390 counts the execution of certain CPU instructions.
> > The root cause is the extremely small run time of the
> > mytest program. It just executes some assembly instructions
> > and then exits. In above invocation the instruction is executed
> > exactly one time (-c1 option). The PMU is expected to report this one
> > time execution by a counter value of one, but fails to do so
> > in some cases, not all.
> >
> > Debugging reveals the invocation of the child process is done
> > *before* the counter events are installed and enabled. Tracing
> > reveals that sometimes the child process starts and exits before
> > the event is installed on all CPUs. The more CPUs the machine has,
> > the more often this miscount happens.
> >
> > Fix this by reversing the start of the work load after the events
> > have been installed on the specified CPUs. Now the comment also
> > matches the code.
> >
> > Output after:
> >  # perf stat -a -e new_pmu/INSTRUCTION_7/ --  mytest -c1 7
> >
> >  Performance counter stats for 'system wide':
> >
> >                  1      new_pmu/INSTRUCTION_7/
> >
> >        0.000366428 seconds time elapsed
> >  #
> >
> > Now the correct result is reported rock solid all the time regardless
> > how many CPUs are online.
> >
> > Fixes:  acf2892270dc ("perf stat: Use perf_evlist__prepare/start_workload())
> >
> > Signed-off-by: Thomas Richter <tmricht@...ux.ibm.com>
> > Acked-by: Sumanth Korikkar <sumanthk@...ux.ibm.com>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > ---
> >  tools/perf/builtin-stat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 3f98689dd687..60baa3dadc4b 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -955,10 +955,10 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >        * Enable counters and exec the command:
> >        */
> >       if (forks) {
> > -             evlist__start_workload(evsel_list);
> >               err = enable_counters();
> >               if (err)
> >                       return -1;
> > +             evlist__start_workload(evsel_list);
>
> right, without -a the event has enable_on_exec so the race does not
> matter, but it's a problem for system wide with fork

Agreed.  Also we may move the enable_counters() and the
clock code out of the if block to be shared with the else block.

Acked-by: Namhyung Kim <namhyung@...nel.org>

Thanks,
Namhyung