lkml.org 
[lkml]   [2018]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] mremap: Avoid TLB flushing anonymous pages that are not in swap cache
From
Date
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.

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?

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?

> + 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.

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.

\
 
 \ /
  Last update: 2018-06-05 20:18    [W:0.183 / U:0.564 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site