Re: [PATCH 2/2] perf stat: Remove nmi watchdog check code again

[Arnaldo Carvalho de Melo]

Em Thu, Jun 09, 2016 at 06:14:39AM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> 
> Now that the NMI watchdog runs with reference cycles, and does not

Now as in when? We should at least warn the user that the kernel used is
one where the NMI watchdog will not get in the way of topdown. Is there
a programmatic way to discover that?

- Arnaldo

> conflict with TopDown anymore, we don't need to check that the
> NMI watchdog is off in perf stat --topdown.
> 
> Remove the code that does this and always use a group unconditionally.
> 
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/Documentation/perf-stat.txt |  6 ------
>  tools/perf/arch/x86/util/Build         |  1 -
>  tools/perf/arch/x86/util/group.c       | 27 ---------------------------
>  tools/perf/builtin-stat.c              | 18 +-----------------
>  tools/perf/util/group.h                |  7 -------
>  5 files changed, 1 insertion(+), 58 deletions(-)
>  delete mode 100644 tools/perf/arch/x86/util/group.c
>  delete mode 100644 tools/perf/util/group.h
> 
> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> index d96ccd4844df..6cf4028ddc2c 100644
> --- a/tools/perf/Documentation/perf-stat.txt
> +++ b/tools/perf/Documentation/perf-stat.txt
> @@ -225,12 +225,6 @@ CPU thread. Per core mode is automatically enabled
>  and -a (global monitoring) is needed, requiring root rights or
>  perf.perf_event_paranoid=-1.
>  
> -Topdown uses the full Performance Monitoring Unit, and needs
> -disabling of the NMI watchdog (as root):
> -echo 0 > /proc/sys/kernel/nmi_watchdog
> -for best results. Otherwise the bottlenecks may be inconsistent
> -on workload with changing phases.
> -
>  This enables --metric-only, unless overriden with --no-metric-only.
>  
>  To interpret the results it is usually needed to know on which
> diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
> index f95e6f46ef0d..bc24b75add88 100644
> --- a/tools/perf/arch/x86/util/Build
> +++ b/tools/perf/arch/x86/util/Build
> @@ -3,7 +3,6 @@ libperf-y += tsc.o
>  libperf-y += pmu.o
>  libperf-y += kvm-stat.o
>  libperf-y += perf_regs.o
> -libperf-y += group.o
>  
>  libperf-$(CONFIG_DWARF) += dwarf-regs.o
>  libperf-$(CONFIG_BPF_PROLOGUE) += dwarf-regs.o
> diff --git a/tools/perf/arch/x86/util/group.c b/tools/perf/arch/x86/util/group.c
> deleted file mode 100644
> index 37f92aa39a5d..000000000000
> --- a/tools/perf/arch/x86/util/group.c
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -#include <stdio.h>
> -#include "api/fs/fs.h"
> -#include "util/group.h"
> -
> -/*
> - * Check whether we can use a group for top down.
> - * Without a group may get bad results due to multiplexing.
> - */
> -bool arch_topdown_check_group(bool *warn)
> -{
> -	int n;
> -
> -	if (sysctl__read_int("kernel/nmi_watchdog", &n) < 0)
> -		return false;
> -	if (n > 0) {
> -		*warn = true;
> -		return false;
> -	}
> -	return true;
> -}
> -
> -void arch_topdown_group_warn(void)
> -{
> -	fprintf(stderr,
> -		"nmi_watchdog enabled with topdown. May give wrong results.\n"
> -		"Disable with echo 0 > /proc/sys/kernel/nmi_watchdog\n");
> -}
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index dff63733dfb7..a599a78b2f22 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -59,10 +59,8 @@
>  #include "util/thread.h"
>  #include "util/thread_map.h"
>  #include "util/counts.h"
> -#include "util/group.h"
>  #include "util/session.h"
>  #include "util/tool.h"
> -#include "util/group.h"
>  #include "asm/bug.h"
>  
>  #include <api/fs/fs.h>
> @@ -1861,16 +1859,6 @@ static int topdown_filter_events(const char **attr, char **str, bool use_group)
>  	return 0;
>  }
>  
> -__weak bool arch_topdown_check_group(bool *warn)
> -{
> -	*warn = false;
> -	return false;
> -}
> -
> -__weak void arch_topdown_group_warn(void)
> -{
> -}
> -
>  /*
>   * Add default attributes, if there were no attributes specified or
>   * if -d/--detailed, -d -d or -d -d -d is used:
> @@ -2010,7 +1998,6 @@ static int add_default_attributes(void)
>  
>  	if (topdown_run) {
>  		char *str = NULL;
> -		bool warn = false;
>  
>  		if (stat_config.aggr_mode != AGGR_GLOBAL &&
>  		    stat_config.aggr_mode != AGGR_CORE) {
> @@ -2025,14 +2012,11 @@ static int add_default_attributes(void)
>  
>  		if (!force_metric_only)
>  			metric_only = true;
> -		if (topdown_filter_events(topdown_attrs, &str,
> -				arch_topdown_check_group(&warn)) < 0) {
> +		if (topdown_filter_events(topdown_attrs, &str, true) < 0) {
>  			pr_err("Out of memory\n");
>  			return -1;
>  		}
>  		if (topdown_attrs[0] && str) {
> -			if (warn)
> -				arch_topdown_group_warn();
>  			err = parse_events(evsel_list, str, NULL);
>  			if (err) {
>  				fprintf(stderr,
> diff --git a/tools/perf/util/group.h b/tools/perf/util/group.h
> deleted file mode 100644
> index 116debe7a995..000000000000
> --- a/tools/perf/util/group.h
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -#ifndef GROUP_H
> -#define GROUP_H 1
> -
> -bool arch_topdown_check_group(bool *warn);
> -void arch_topdown_group_warn(void);
> -
> -#endif
> -- 
> 2.5.5