Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220317155346.577384-1-tmricht@linux.ibm.com> <Yjr/X+Cuunyo84S3@krava>
In-Reply-To: <Yjr/X+Cuunyo84S3@krava>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Wed, 23 Mar 2022 13:54:23 -0700
Message-ID: <CAM9d7cgkwDQCEWRFT2G87fba7MLPR2OtEn6pvHDzpegpR-AMSA@mail.gmail.com>
Subject: Re: [PATCH] perf/stat: Fix perf stat for forked applications
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     Thomas Richter <tmricht@linux.ibm.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-perf-users <linux-perf-users@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        gor@linux.ibm.com, Arnaldo Carvalho de Melo <acme@redhat.com>,
        Sven Schnelle <svens@linux.ibm.com>, sumanthk@linux.ibm.com,
        Heiko Carstens <hca@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 23, 2022 at 4:07 AM Jiri Olsa <olsajiri@gmail.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@linux.ibm.com>
> > Acked-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
> > Cc: Namhyung Kim <namhyung@kernel.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@kernel.org>

Thanks,
Namhyung