Re: [PATCH 1/1] perf,tools: add time out to force stop endless mmap processing
From: Arnaldo Carvalho de Melo
Date: Tue Jun 16 2015 - 14:09:02 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/