Re: [PATCHES/RFC] Re: A concern about overflow ring buffer mode

From: Liang, Kan
Date: Mon Oct 29 2018 - 10:33:11 EST

On 10/29/2018 9:03 AM, Arnaldo Carvalho de Melo wrote:
Em Fri, Oct 26, 2018 at 04:11:51PM -0400, Liang, Kan escreveu:
On 10/26/2018 3:24 PM, Arnaldo Carvalho de Melo wrote:
Em Fri, Oct 26, 2018 at 03:16:29PM -0400, Liang, Kan escreveu:
On 10/26/2018 3:12 PM, Arnaldo Carvalho de Melo wrote:
Em Fri, Oct 26, 2018 at 03:07:40PM -0400, Liang, Kan escreveu:
On 10/26/2018 3:02 PM, Arnaldo Carvalho de Melo wrote:
I checked and both have the same result. But I still think there is
value in having the shorter form, ok?



I think that we should default back to --no-overwrite till we get this
sorted out, as the effect is easily noticeable, as David reported and I
reproduced, when doing kernel builds.
It is mainly for performance reason to switch to overwrite mode. The impact
was very small when I did my test. But now the effect is easily noticeable
in other tests. Yes, I agree. We may change it back to non-overwrite mode
until the issue is addressed.

So, I have these two patches in my perf/core branch, with Fixes tags
that will make them get to the stable kernels, ok?

I just realized that the problem in KNL will be back if we switch back to non-overwrite mode.
The problem is that users have to wait tens of minutes to see perf top results on the screen in KNL. Before that, there is nothing but a black screen.

Sorry I didn't notice it last Friday. Because I thought the ui_warning in perf_top__mmap_read() can give user a hint. So the user can switch to overwrite mode manually.
But unfortunately, the ui_warning doesn't work. Because it is called after perf_top__mmap_read(). The processing time of perf_top__mmap_read() could be tens of minutes.


From f54ef0e7342efb77205b2faaacbcb81cdd31f064 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Date: Fri, 26 Oct 2018 15:55:23 -0300
Subject: [PATCH 1/2] perf top: Allow disabling the overwrite mode

In ebebbf082357 ("perf top: Switch default mode to overwrite mode") we
forgot to leave a way to disable that new default, add a --overwrite
option that can be disabled using --no-overwrite, since the code already
in such a way that we can readily disable this mode.

This is useful when investigating bugs with this mode like the recent
report from David Miller where lots of unknown symbols appear due to
disabling the events while processing them which disables all record
types, not just PERF_RECORD_SAMPLE, which makes it impossible to resolve
maps when we lose PERF_RECORD_MMAP records.

This can be easily seen while building a kernel, when there are lots of
short lived processes.

Reported-by: David Miller <davem@xxxxxxxxxxxxx>
Acked-by: Kan Liang <kan.liang@xxxxxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Cc: David Ahern <dsahern@xxxxxxxxx>
Cc: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Wang Nan <wangnan0@xxxxxxxxxx>
Fixes: ebebbf082357 ("perf top: Switch default mode to overwrite mode")
Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
tools/perf/Documentation/perf-top.txt | 5 +++++
tools/perf/builtin-top.c | 2 ++
2 files changed, 7 insertions(+)

diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index 114fda12aa49..d4be6061fe1c 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -242,6 +242,11 @@ Default is to monitor all CPUS.
Enable hierarchy output.
+ This is the default, but for investigating problems with it or any other strange
+ behaviour like lots of unknown samples, we may want to disable this mode by using
+ --no-overwrite.
Don't do ownership validation.
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index d21d8751e749..214fad747b04 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1372,6 +1372,8 @@ int cmd_top(int argc, const char **argv)
"Show raw trace event output (do not use print fmt or plugins)"),
OPT_BOOLEAN(0, "hierarchy", &symbol_conf.report_hierarchy,
"Show entries in a hierarchy"),
+ OPT_BOOLEAN(0, "overwrite", &top.record_opts.overwrite,
+ "Use a backward ring buffer, default: yes"),
OPT_BOOLEAN(0, "force", &symbol_conf.force, "don't complain, do it"),
OPT_UINTEGER(0, "num-thread-synthesize", &top.nr_threads_synthesize,
"number of thread to run event synthesize"),