Messages in this thread Patch in this message | | | Date | Tue, 16 Jun 2015 15:08:27 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 1/1] perf,tools: add time out to force stop endless mmap processing |
| |
Em Tue, Jun 16, 2015 at 04:42:49PM +0000, Liang, Kan escreveu: > > 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
Sure, that would be overkill, way annoying and completely unnecessary...
> do anything about this warning. So pr_warning should be good enough.
I think it is the right interface, its only problem when used with the TUI is that it doesn't go to a file that could be accessible via a hotkey in a TUI pager, searchable, etc, like less on stdio. > Regarding to timeout value, I will add a per-thread-proc-map-processing > timeout parameter in next version. The default value will be 50ms.
Ok. > 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
Right, looks ok if we want to do this without passing an extra "synthesize_stats" parameter that we would look at the end of the synthesizing on a perf_event__warn_about_synth_errors() that would receive this synthesize_stats struct. > @@ -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
| |