Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0770187827F@SHSMSX103.ccr.corp.intel.com>
Date:	Tue, 16 Jun 2015 16:42:49 +0000
From:	"Liang, Kan" <kan.liang@...el.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	David Ahern <dsahern@...il.com>
CC:	Andi Kleen <andi@...stfloor.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Huang, Ying" <ying.huang@...el.com>
Subject: RE: [PATCH 1/1] perf,tools: add time out to force stop endless mmap
 processing



> Em Fri, Jun 12, 2015 at 10:24:36PM -0600, David Ahern escreveu:
> > coming back to this ...
> 
> > On 6/12/15 2:39 PM, Liang, Kan wrote:
> > >>>Yes, perf always can read proc file. The problem is that the proc
> > >>>file is huge and keep growing faster than proc reader.
> > >>>So perf top do loop in perf_event__synthesize_mmap_events until
> the
> > >>>test case exit.
> 
> > >>I'm confused. How are you getting the above time to read /proc maps
> > >>if it never finishes?
> 
> > >I just tried to simplify the issue for perf record. So you may
> > >noticed that I only read one thread. There are several threads in the
> system.
> > >Also, I do the perf record test when starting the test case.
> > >The proc file is not that big.
> > >For perf top, it will monitor whole system. So it never finishes.
> >
> > If the proc file is not that big for perf-record why is it a problem
> > for perf-top? Both should only be reading the maps file for the thread
> > group leader once and after it is processed getting MMAP events for
> > changes. Why do you say perf-top can't handle it but perf-record can?
> 
> 'perf top' does more than 'perf record', so it is conceivable that in some
> circumstances 'perf record' can go thru, while top struggles.
> 
> That being said this happens when synthesizing PERF_RECORD_ events for
> existing threads, i.e. at tool start time, for both top and record, so, for this
> specific case, there should be no difference, if the workloads running in
> both cases are the same at tool start up phase.
> 
> Then, that being said, having a sane upper limit on the time for processing
> those events makes the tool more robust and allows it to do most of its
> work, just samples for the maps not synthesized will fail to get resolved to
> symbols/DSOs.
> 
> For those cases we should, during synthesizing, do both what Kan did in his
> patch, i.e. emit a log warning with the COMM/PID that we are truncating
> /proc/PID/maps parsing, and increment a counter that, just after we finish
> synthesizing we should report, in a similar way as we do in
> perf_session__warn_about_errors() after processing events, something
> like:
> 
>         +--------------------------------------------------------+
>         | %d map information files for pre-existing threads were |
>         | not processed, if there are samples for addresses they |
>         | will not be resolved, you may find out which are these |
>         | threads by running with -v and redirecting the output  |
>         | to a file.                                             |
>         +--------------------------------------------------------+
> 
> Ideally, as an extra step, we could flip a flag on the 'struct thread'
> where these maps got truncated and add some visual cue to the hist_entry
> instances (lines in the top UI).
> 
> Perhaps we should add a per-thread-proc-map-processing timeout
> parameter to the synthesizing routines instead of having that hardcoded,
> i.e.
> allow the tool to specify what is reasonable for it, but that wouldn't be
> strictly required for a first patch, emitting the dialog box above after
> synthesizing, if truncation happened, is.
> 
> Agreed?

Yes, we can print the warning in perf_session__warn_about_errors,
so the user will get warning from both perf record and perf report.

perf top will not call perf_session__warn_about_errors. So I think I
will still use pr_warning in V2 patch to notify user. Because if we use
ui__warning, the user has to press any key to close the dialog box.
In my test, I have 48 threads with huge maps. It feels awful to press
48 space to close warning box. Furthermore, as Andi said the user cannot
do anything about this warning. So  pr_warning should be good enough.

Regarding to timeout value, I will add a per-thread-proc-map-processing
timeout parameter in next version. The default value will be 50ms. 

We still need a way to notify the perf report that some map is incomplete.
I plan to add a bit PERF_RECORD_MISC_MMAP_TIME_OUT (1 << 12) for
event->header.misc.
When timeout detect, it generates a MMAP2 record as below:
The perf tool will check the bit to know which mmap is incomplete and
update evlist-status for perf_session__warn_about_errors

@@ -253,6 +258,11 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
                if (fgets(bf, sizeof(bf), fp) == NULL)
                        break;

+               if ((rdclock() - t) > MMAP_TIMEOUT) {
+                       timeout = true;
+                       goto out;
+               }
+
                /* ensure null termination since stack will be reused. */
                strcpy(execname, "");

@@ -301,6 +311,10 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
                        event->header.misc |= PERF_RECORD_MISC_MMAP_DATA;
                }

+out:
+               if (timeout)
+                       event->header.misc |= PERF_RECORD_MISC_MMAP_TIME_OUT;
+
                if (!strcmp(execname, ""))
                        strcpy(execname, anonstr);

@@ -319,6 +333,9 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
                        rc = -1;
                        break;
                }
+
+               if (timeout)
+                       break;
        }

        fclose(fp);


Thanks,
Kan

> 
> - Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists