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.
