Re: [PATCH 1/1] perf,tools: add time out to force stop endless mmap processing

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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