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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bfc2e579-915f-24db-0ff0-29bd9148b8c0@intel.com>
Date:   Tue, 5 Jun 2018 11:18:18 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Mel Gorman <mgorman@...hsingularity.net>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     mhocko@...nel.org, vbabka@...e.cz, Aaron Lu <aaron.lu@...el.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] mremap: Avoid TLB flushing anonymous pages that are not
 in swap cache

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.