Re: [PATCH] mm: release the spinlock on zap_pte_range

[Michal Hocko <mhocko@kernel.org>]

[Cc Nick - the email thread starts http://lkml.kernel.org/r/20190729071037.241581-1-minchan@kernel.org
 A very brief summary is that mark_page_accessed seems to be quite
 expensive and the question is whether we still need it and why
 SetPageReferenced cannot be used instead. More below.]

On Tue 30-07-19 21:39:35, Minchan Kim wrote:
> On Tue, Jul 30, 2019 at 02:32:37PM +0200, Michal Hocko wrote:
> > On Tue 30-07-19 21:11:10, Minchan Kim wrote:
> > > On Mon, Jul 29, 2019 at 10:35:15AM +0200, Michal Hocko wrote:
> > > > On Mon 29-07-19 17:20:52, Minchan Kim wrote:
> > > > > On Mon, Jul 29, 2019 at 09:45:23AM +0200, Michal Hocko wrote:
> > > > > > On Mon 29-07-19 16:10:37, Minchan Kim wrote:
> > > > > > > In our testing(carmera recording), Miguel and Wei found unmap_page_range
> > > > > > > takes above 6ms with preemption disabled easily. When I see that, the
> > > > > > > reason is it holds page table spinlock during entire 512 page operation
> > > > > > > in a PMD. 6.2ms is never trivial for user experince if RT task couldn't
> > > > > > > run in the time because it could make frame drop or glitch audio problem.
> > > > > > 
> > > > > > Where is the time spent during the tear down? 512 pages doesn't sound
> > > > > > like a lot to tear down. Is it the TLB flushing?
> > > > > 
> > > > > Miguel confirmed there is no such big latency without mark_page_accessed
> > > > > in zap_pte_range so I guess it's the contention of LRU lock as well as
> > > > > heavy activate_page overhead which is not trivial, either.
> > > > 
> > > > Please give us more details ideally with some numbers.
> > > 
> > > I had a time to benchmark it via adding some trace_printk hooks between
> > > pte_offset_map_lock and pte_unmap_unlock in zap_pte_range. The testing
> > > device is 2018 premium mobile device.
> > > 
> > > I can get 2ms delay rather easily to release 2M(ie, 512 pages) when the
> > > task runs on little core even though it doesn't have any IPI and LRU
> > > lock contention. It's already too heavy.
> > > 
> > > If I remove activate_page, 35-40% overhead of zap_pte_range is gone
> > > so most of overhead(about 0.7ms) comes from activate_page via
> > > mark_page_accessed. Thus, if there are LRU contention, that 0.7ms could
> > > accumulate up to several ms.
> > 
> > Thanks for this information. This is something that should be a part of
> > the changelog. I am sorry to still poke into this because I still do not
> 
> I will include it.
> 
> > have a full understanding of what is going on and while I do not object
> > to drop the spinlock I still suspect this is papering over a deeper
> > problem.
> 
> I couldn't come up with better solution. Feel free to suggest it.
> 
> > 
> > If mark_page_accessed is really expensive then why do we even bother to
> > do it in the tear down path in the first place? Why don't we simply set
> > a referenced bit on the page to reflect the young pte bit? I might be
> > missing something here of course.
> 
> commit bf3f3bc5e73
> Author: Nick Piggin <npiggin@suse.de>
> Date:   Tue Jan 6 14:38:55 2009 -0800
> 
>     mm: don't mark_page_accessed in fault path
> 
>     Doing a mark_page_accessed at fault-time, then doing SetPageReferenced at
>     unmap-time if the pte is young has a number of problems.
> 
>     mark_page_accessed is supposed to be roughly the equivalent of a young pte
>     for unmapped references. Unfortunately it doesn't come with any context:
>     after being called, reclaim doesn't know who or why the page was touched.
> 
>     So calling mark_page_accessed not only adds extra lru or PG_referenced
>     manipulations for pages that are already going to have pte_young ptes anyway,
>     but it also adds these references which are difficult to work with from the
>     context of vma specific references (eg. MADV_SEQUENTIAL pte_young may not
>     wish to contribute to the page being referenced).
> 
>     Then, simply doing SetPageReferenced when zapping a pte and finding it is
>     young, is not a really good solution either. SetPageReferenced does not
>     correctly promote the page to the active list for example. So after removing
>     mark_page_accessed from the fault path, several mmap()+touch+munmap() would
>     have a very different result from several read(2) calls for example, which
>     is not really desirable.

Well, I have to say that this is rather vague to me. Nick, could you be
more specific about which workloads do benefit from this change? Let's
say that the zapped pte is the only referenced one and then reclaim
finds the page on inactive list. We would go and reclaim it. But does
that matter so much? Hot pages would be referenced from multiple ptes
very likely, no?

That being said, cosindering that mark_page_accessed is not free, do we
have strong reasons to keep it?

Or do I miss something?

>     Signed-off-by: Nick Piggin <npiggin@suse.de>
>     Acked-by: Johannes Weiner <hannes@saeurebad.de>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

-- 
Michal Hocko
SUSE Labs