Re: [PATCH] mremap: Avoid TLB flushing anonymous pages that are not in swap cache

[Mel Gorman <mgorman@techsingularity.net>]

On Tue, Jun 05, 2018 at 11:18:18AM -0700, Dave Hansen wrote:
> On 06/05/2018 10:13 AM, Mel Gorman wrote:
> > The anonymous page race fix is overkill for two reasons. Pages that are not
> > in the swap cache are not going to be issued for IO and if a stale TLB entry
> > is used, the write still occurs on the same physical page. Any race with
> > mmap replacing the address space is handled by mmap_sem. As anonymous pages
> > are often dirty, it can mean that mremap always has to flush even when it is
> > not necessary.
> 
> This looks fine to me.  One nit on the description: I found myself
> wondering if we skip the flush under the ptl where the flush is
> eventually done.  That code is a bit out of the context, so we don't see
> it in the patch.
> 

That's fair enough. I updated part of the changelog to read

This patch special cases anonymous pages to only flush ranges under the
page table lock if the page is in swap cache and can be potentially queued
for IO. Note that the full flush of the range being mremapped is still
flushed so TLB flushes are not eliminated entirely.

Does that work for you?

> We have two modes of flushing during move_ptes():
> 1. The flush_tlb_range() while holding the ptl in move_ptes().
> 2. A flush_tlb_range() at the end of move_table_tables(), driven by
>   'need_flush' which will be set any time move_ptes() does *not* flush.
> 
> This patch broadens the scope where move_ptes() does not flush and
> shifts the burden to the flush inside move_table_tables().
> 
> Right?
> 

Yes. While this does not eliminate TLB flushes, it reduces the number
considerably as we potentially are replacing one-flush-per-LATENCY_LIMIT
with one flush.

> Other minor nits:
> 
> > +/* Returns true if a TLB must be flushed before PTL is dropped */
> > +static bool should_force_flush(pte_t *pte)
> > +{
> 
> I usually try to make the non-pte-modifying functions take a pte_t
> instead of 'pte_t *' to make it obvious that there no modification going
> on.  Any reason not to do that here?
> 

No, it was just a minor saving on stack usage.

> > +	if (!trylock_page(page))
> > +		return true;
> > +	is_swapcache = PageSwapCache(page);
> > +	unlock_page(page);
> > +
> > +	return is_swapcache;
> > +}
> 
> I was hoping we didn't have to go as far as taking the page lock, but I
> guess the proof is in the pudding that this tradeoff is worth it.
> 

In the interest of full disclosure, the feedback I have from the customer is
based on a patch that modifies the LATENCY_LIMIT. This helped but did not
eliminate the problem. This was potentially a better solution but I wanted
review first. I'm waiting on confirmation this definitely behaves better.

> BTW, do you want to add a tiny comment about why we do the
> trylock_page()?  I assume it's because we don't want to wait on finding
> an exact answer: we just assume it is in the swap cache if the page is
> locked and flush regardless.

It's really because calling lock_page while holding a spinlock is
eventually going to ruin your day.

Thanks.

-- 
Mel Gorman
SUSE Labs