Re: perf synthesized mmap timeouts

[Liang, Kan]

On 10/28/2018 12:41 AM, David Miller wrote:
> If I understand the commit message for:
>
> commit 8cc42de736b617827a4e7664fb8d7a325bc125bc
> Author: Kan Liang <kan.liang@xxxxxxxxx>
> Date:   Thu Jan 18 13:26:32 2018 -0800
>
>      perf top: Check the latency of perf_top__mmap_read()
>
> properly, the problem is that a malicious or out of control
> app can be doing endless mmaps causing perf to loop forever
> processing the /proc/$PID/maps file.

NO, the perf_top__mmap_read() is used to read and process all available 
samples on each ringbuffer. It will be called repeatedly until perf top 
exit.

If it is a fully loaded system, the processing time of the function will 
be very long. Even much longer than refresh time for display thread. If 
so, it means that only stale data can be shown, or even worse, nothing 
can be shown.
For example, in the Knights Landing/Mill platform, the processing time 
could be tens of minutes if there is a heavy load (kernel building).
Because for the Knights Landing/Mill platform, the CPU# is big (> 200), 
but the computational capabilities for each core is weak (Atom core).
There is nothing shown on the screen during this period.
That's why we add a warning here to give user a hint.

If it's annoying for your case, I think you encountered the similar 
problem. You may want to follow the hint, and have the latest data 
refreshed. On the other hand, I agree that we need a more decent way to 
deliver the hint.

Thanks,
Kan

> But that is not what this commit is handling at all.
>
> It is instead applying a large hammer which quits if it is taking a
> long time to process the maps, not if the process's mmap list is
> growing endlessly while we process it.
>
> This triggers any time I run perf top on a fully loaded system making
> perf less useful than it should be.
>
> And it triggers simply because the perf synthesize threads have to
> share the cpu with the workload already running.
>
> So it takes more than half a second to process emacs's 527 maps when
> the number of running processes is ~NCPUS?  Big deal.  We should let
> it finish....
>
> The tradeoff choosen here is really bad.
>
> Guess what happens if you don't have maps for a given process?
>
> What happens is that for every single sample we get within that range,
> we get a completely unique histogram entry.
>
> This means potentially millions and millions of histogram entries
> where there should only be a few hundred.
>
> This makes the histogram rbtree huge, and slow to process.
>
> So not only is top unable to provide correct histogram output, it is
> also running sluggishly.
>
> A way to mitigate the actual problem would be to snapshot the maps
> file into a large buffer, if possible.  We can get the full contents
> faster than the process in question can make more maps.  At most we
> will do one additional read at the end if they were able to sneak in
> one new mmap during the initial read.
>
> No timeout necessary.  We have the complete maps file, our processing
> time is therefore bounded.
>
> Thanks.