Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754708AbbFPSIx (ORCPT ); Tue, 16 Jun 2015 14:08:53 -0400 Received: from mail.kernel.org ([198.145.29.136]:37137 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751404AbbFPSIp (ORCPT ); Tue, 16 Jun 2015 14:08:45 -0400 Date: Tue, 16 Jun 2015 15:08:27 -0300 From: Arnaldo Carvalho de Melo To: "Liang, Kan" Cc: David Ahern , Andi Kleen , "linux-kernel@vger.kernel.org" , "Huang, Ying" Subject: Re: [PATCH 1/1] perf,tools: add time out to force stop endless mmap processing Message-ID: <20150616180827.GH10374@kernel.org> References: <37D7C6CF3E00A74B8858931C1DB2F077018767AD@SHSMSX103.ccr.corp.intel.com> <557AFDB1.7030902@gmail.com> <37D7C6CF3E00A74B8858931C1DB2F07701876834@SHSMSX103.ccr.corp.intel.com> <557B16C4.7000000@gmail.com> <37D7C6CF3E00A74B8858931C1DB2F07701876CB2@SHSMSX103.ccr.corp.intel.com> <557B3309.4080002@gmail.com> <37D7C6CF3E00A74B8858931C1DB2F07701876D60@SHSMSX103.ccr.corp.intel.com> <557BB084.8030800@gmail.com> <20150616151159.GF10374@kernel.org> <37D7C6CF3E00A74B8858931C1DB2F0770187827F@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F0770187827F@SHSMSX103.ccr.corp.intel.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6413 Lines: 146 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/