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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Jun 09, 2016 at 06:14:39AM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Now that the NMI watchdog runs with reference cycles, and does not
> 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.

PeterZ, will you pick this one? I'm ok with it, Andi is just removing
some code he introduced recently.

- Arnaldo
 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  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