Date:   Thu, 23 Feb 2023 10:45:13 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Namhyung Kim <namhyung@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Changbin Du <changbin.du@huawei.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ian Rogers <irogers@google.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hui Wang <hw.huiwang@huawei.com>
Subject: Re: [PATCH] perf: fix counting when initial delay configured
Message-ID: <Y/dt6QpsCj5AAUE/@kernel.org>
References: <20230223075800.1795777-1-changbin.du@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230223075800.1795777-1-changbin.du@huawei.com>
Precedence: bulk

Em Thu, Feb 23, 2023 at 03:58:00PM +0800, Changbin Du escreveu:
> When creating counters with initial delay configured, the enable_on_exec
> field is not set. So we need to enable the counters later. The problem
> is, when a workload is specified the target__none() is still true. So
> we also need to check stat_config.initial_delay.
> 
> Before this fix the event is not counted:
> $ ./perf stat -e instructions -D 100 sleep 2
> Events disabled
> Events enabled
> 
>  Performance counter stats for 'sleep 2':
> 
>      <not counted>      instructions
> 
>        1.901661124 seconds time elapsed
> 
>        0.001602000 seconds user
>        0.000000000 seconds sys
> 
> After fix it works:
> $ ./perf stat -e instructions -D 100 sleep 2
> Events disabled
> Events enabled
> 
>  Performance counter stats for 'sleep 2':
> 
>            404,214      instructions
> 
>        1.901743475 seconds time elapsed
> 
>        0.001617000 seconds user
>        0.000000000 seconds sys
> 
> Fixes: c587e77e100f ("perf stat: Do not delay the workload with --delay")

Yeap, even the comment states that we need to enable when initial_delay
is set :-)

I added the additional test output below.

Namhyung, can you please ack it?

- Arnaldo

Committer testing:

Before:

Lets use stress-ng so that we have lots of samples using a CPU stressor
and also intermingle the workload output with the messages about when
the events get enabled (i.e. later on in the workload):

  $ perf stat -e instructions -D 100 stress-ng -c 32 -t 1
  Events disabled
  stress-ng: info:  [38361] setting to a 1 second run per stressor
  stress-ng: info:  [38361] dispatching hogs: 32 cpu
  Events enabled
  stress-ng: info:  [38361] successful run completed in 1.01s

   Performance counter stats for 'stress-ng -c 32 -t 1':

       <not counted>      instructions:u

         0.916479141 seconds time elapsed

        30.868003000 seconds user
         0.049851000 seconds sys


  Some events weren't counted. Try disabling the NMI watchdog:
        echo 0 > /proc/sys/kernel/nmi_watchdog
        perf stat ...
        echo 1 > /proc/sys/kernel/nmi_watchdog
  $

After the fix:

  $ perf stat -e instructions -D 100 stress-ng -c 32 -t 1
  Events disabled
  stress-ng: info:  [40429] setting to a 1 second run per stressor
  stress-ng: info:  [40429] dispatching hogs: 32 cpu
  Events enabled
  stress-ng: info:  [40429] successful run completed in 1.01s

   Performance counter stats for 'stress-ng -c 32 -t 1':

        154117865145      instructions:u

         0.920827644 seconds time elapsed

        30.864753000 seconds user
         0.073862000 seconds sys


  $

> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
>  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 9f3e4b257516..c71d85577de6 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -544,7 +544,7 @@ static int enable_counters(void)
>  	 * - we don't have tracee (attaching to task or cpu)
>  	 * - we have initial delay configured
>  	 */
> -	if (!target__none(&target)) {
> +	if (!target__none(&target) || stat_config.initial_delay) {
>  		if (!all_counters_use_bpf)
>  			evlist__enable(evsel_list);
>  	}
> -- 
> 2.25.1
> 

-- 

- Arnaldo