Re: [PATCH 2/2] perf stat: Disable HW watchdog around a perf stat session

[Arnaldo Carvalho de Melo]

Em Tue, Feb 07, 2017 at 02:10:28AM +0100, Borislav Petkov escreveu:
> From: Borislav Petkov <bp@xxxxxxx>
> 
> When using perf stat on an AMD F15h system with the default hw events
> attributes, some of the events don't get counted:
> 
>  Performance counter stats for 'sleep 1':
> 
>           0.749208      task-clock (msec)         #    0.001 CPUs utilized
>                  1      context-switches          #    0.001 M/sec
>                  0      cpu-migrations            #    0.000 K/sec
>                 54      page-faults               #    0.072 M/sec
>          1,122,815      cycles                    #    1.499 GHz
>            286,740      stalled-cycles-frontend   #   25.54% frontend cycles idle
>      <not counted>      stalled-cycles-backend                                        (0.00%)
>      ^^^^^^^^^^^^
>      <not counted>      instructions                                                  (0.00%)
>      ^^^^^^^^^^^^
>      <not counted>      branches                                                      (0.00%)
>      <not counted>      branch-misses                                                 (0.00%)
> 
>        1.001550070 seconds time elapsed
> 
> The reason is that we have the HW watchdog consume one PMU counter
> and when perf tries to schedule 6 events on 6 counters and some of
> those counters are constrained to only a specific subset of PMCs by the
> hardware, the event scheduling fails.
> 
> So let's disable the HW watchdog around a perf stat session running as
> root and restore it after it to its previous state. This frees up the
> one counter and the scheduling of the default events succeeds:
> 
>  Performance counter stats for 'sleep 1':
> 
>           0.806902      task-clock (msec)         #    0.001 CPUs utilized
>                  1      context-switches          #    0.001 M/sec
>                  0      cpu-migrations            #    0.000 K/sec
>                 55      page-faults               #    0.068 M/sec
>          1,200,677      cycles                    #    1.488 GHz
>            308,044      stalled-cycles-frontend   #   25.66% frontend cycles idle
>            424,292      stalled-cycles-backend    #   35.34% backend cycles idle
>            672,694      instructions              #    0.56  insn per cycle
>                                                   #    0.63  stalled cycles per insn
>            132,965      branches                  #  164.785 M/sec
>              7,300      branch-misses             #    5.49% of all branches
> 
>        1.001689739 seconds time elapsed
> 
> There's a --dont-disable-hwdt option which preserves the old behavior.
> 
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> ---
>  tools/perf/builtin-stat.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a02f2e965628..b2aa2ed3161c 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -146,6 +146,8 @@ static aggr_get_id_t		aggr_get_id;
>  static bool			append_file;
>  static const char		*output_name;
>  static int			output_fd;
> +static bool			keep_hwdt;
> +static int			prev_hwdt;	/* previous HW watchdog state */
>  
>  struct perf_stat {
>  	bool			 record;
> @@ -1539,6 +1541,39 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
>  	fflush(stat_config.output);
>  }
>  
> +static void perf_stat_toggle_hwdt(int on)
> +{
> +	static const char *p = "sys/kernel/nmi_watchdog";
   	static const char *p = "kernel/nmi_watchdog";
> +	int val;
> +
> +	if (keep_hwdt)
> +		return;
> +
> +	if (geteuid())
> +		return;
> +
> +	if (procfs__read_int(p, &val) < 0)
	    sysctl__read_int(p, &val) < 0)
> +		return;
> +
> +	/* Reenable only when it was enabled before. */
> +	if (on) {
> +		if (prev_hwdt)
> +			goto write;
> +	/* Disable HWDT only when it is enabled. */
> +	} else {
> +		prev_hwdt = val;
> +
> +		if (val)
> +			goto write;
> +	}
> +
> +	return;
> +
> +write:
> +	if (procfs__write_int(p, on) < 0)
> +		return;
> +}
> +
>  static volatile int signr = -1;
>  
>  static void skip_signal(int signo)
> @@ -1575,6 +1610,8 @@ static void sig_atexit(void)
>  
>  	sigprocmask(SIG_SETMASK, &oset, NULL);
>  
> +	perf_stat_toggle_hwdt(1);
> +
>  	if (signr == -1)
>  		return;
>  
> @@ -1659,6 +1696,8 @@ static const struct option stat_options[] = {
>  			"Only print computed metrics. No raw values", enable_metric_only),
>  	OPT_BOOLEAN(0, "topdown", &topdown_run,
>  			"measure topdown level 1 statistics"),
> +	OPT_BOOLEAN(0, "dont-disable-hwdt", &keep_hwdt,
> +			"Do not disable HW NMI watchdog during the current session"),
>  	OPT_END()
>  };
>  
> @@ -2523,6 +2562,8 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (perf_stat_init_aggr_mode())
>  		goto out;
>  
> +	perf_stat_toggle_hwdt(0);
> +
>  	/*
>  	 * We dont want to block the signals - that would cause
>  	 * child tasks to inherit that and Ctrl-C would not work.
> -- 
> 2.11.0
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.