Messages in this thread | | | Subject | Re: [PATCH] mremap: Avoid TLB flushing anonymous pages that are not in swap cache | From | Dave Hansen <> | Date | Tue, 5 Jun 2018 11:18:18 -0700 |
| |
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.
| |