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

[Liang, Kan]

On 10/29/2018 10:35 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 29, 2018 at 10:33:06AM -0400, Liang, Kan escreveu:
> > 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:
> > > > 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.
>
> So we need a way to notice that we're in a machine like that and warn
> the user before the wait takes place, ideas on how to do that?
The processing time for each perf_top__mmap_read_idx() should not that 
long. We may check it after each perf_top__mmap_read_idx(). Also change 
the ui_warning to one-time warning. The patch as below can do that (not 
test).


diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index d21d875..5e532e0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -877,31 +877,40 @@ static void perf_top__mmap_read_idx(struct 
perf_top *top, int idx)
 	perf_mmap__read_done(md);
 }

+static bool check_processing_time = true;
+
 static void perf_top__mmap_read(struct perf_top *top)
 {
 	bool overwrite = top->record_opts.overwrite;
 	struct perf_evlist *evlist = top->evlist;
-	unsigned long long start, end;
+	unsigned long long start, end, tolerance;
 	int i;

-	start = rdclock();
 	if (overwrite)
 		perf_evlist__toggle_bkw_mmap(evlist, BKW_MMAP_DATA_PENDING);

-	for (i = 0; i < top->evlist->nr_mmaps; i++)
+	tolerance = (unsigned long long)top->delay_secs * NSEC_PER_SEC / 
top->evlist->nr_mmaps;
+	start = rdclock();
+	for (i = 0; i < top->evlist->nr_mmaps; i++) {
 		perf_top__mmap_read_idx(top, i);
+		if (check_processing_time) {
+			end = rdclock();
+
+			if ((end - start) > tolerance) {
+				ui__warning("Too slow to read ring buffer.\n"
+					    "Please try increasing the period (-c) or\n"
+					    "decreasing the freq (-F) or\n"
+					    "limiting the number of CPUs (-C)\n");
+				check_processing_time = false;
+			}
+			start = end;
+		}
+	}

 	if (overwrite) {
 		perf_evlist__toggle_bkw_mmap(evlist, BKW_MMAP_EMPTY);
 		perf_evlist__toggle_bkw_mmap(evlist, BKW_MMAP_RUNNING);
 	}
-	end = rdclock();
-
-	if ((end - start) > (unsigned long long)top->delay_secs * NSEC_PER_SEC)
-		ui__warning("Too slow to read ring buffer.\n"
-			    "Please try increasing the period (-c) or\n"
-			    "decreasing the freq (-F) or\n"
-			    "limiting the number of CPUs (-C)\n");
 }

 /*