Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Hansen <dave.hansen@intel.com>
To: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@kernel.org>, Jann Horn <jannh@google.com>,
	Linux-MM <linux-mm@kvack.org>,
	kernel list <linux-kernel@vger.kernel.org>,
	Daniel Colascione <dancol@google.com>,
	"Joel Fernandes (Google)" <joel@joelfernandes.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: interaction of MADV_PAGEOUT with CoW anonymous mappings?
Date: Fri, 13 Mar 2020 09:59:50 -0700
Message-ID: <a3a8a428-17d3-e3cb-913c-b44de12db9e4@intel.com> (raw)
In-Reply-To: <20200313020018.GC68817@google.com>

On 3/12/20 7:00 PM, Minchan Kim wrote:
> On Thu, Mar 12, 2020 at 02:41:07PM -0700, Dave Hansen wrote:
>> One other fun thing.  I have a "victim" thread sitting in a loop doing:
>>
>> 	sleep(1)
>> 	memcpy(&garbage, buffer, sz);
>>
>> The "attacker" is doing
>>
>> 	madvise(buffer, sz, MADV_PAGEOUT);
>>
>> in a loop.  That, oddly enough doesn't cause the victim to page fault.
>> But, if I do:
>>
>> 	memcpy(&garbage, buffer, sz);
>> 	madvise(buffer, sz, MADV_PAGEOUT);
>>
>> It *does* cause the memory to get paged out.  The MADV_PAGEOUT code
>> actually has a !pte_present() check.  It will punt on a PTE if it sees
>> it.  In other words, if a page is in the swap cache but not mapped by a
>> pte_present() PTE, MADV_PAGEOUT won't touch it.
>>
>> Shouldn't MADV_PAGEOUT be able to find and reclaim those pages?  Patch
>> attached.
> 
>>
>>
>> ---
>>
>>  b/mm/madvise.c |   38 +++++++++++++++++++++++++++++++-------
>>  1 file changed, 31 insertions(+), 7 deletions(-)
>>
>> diff -puN mm/madvise.c~madv-pageout-find-swap-cache mm/madvise.c
>> --- a/mm/madvise.c~madv-pageout-find-swap-cache	2020-03-12 14:24:45.178775035 -0700
>> +++ b/mm/madvise.c	2020-03-12 14:35:49.706773378 -0700
>> @@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s
>>  #endif		/* CONFIG_SWAP */
>>  
>>  /*
>> + * Given a PTE, find the corresponding 'struct page'.  Also handles
>> + * non-present swap PTEs.
>> + */
>> +struct page *pte_to_reclaim_page(struct vm_area_struct *vma,
>> +				 unsigned long addr, pte_t ptent)
>> +{
>> +	swp_entry_t entry;
>> +
>> +	/* Totally empty PTE: */
>> +	if (pte_none(ptent))
>> +		return NULL;
>> +
>> +	/* A normal, present page is mapped: */
>> +	if (pte_present(ptent))
>> +		return vm_normal_page(vma, addr, ptent);
>> +
> 
> Please check is_swap_pte first.

Why?

is_swap_pte() duplicates the first two checks.  But, I need an explicit
pte_present() check somewhere because I need to call vm_normal_page()
only on present PTEs.

I guess the pte_present() check could be:

	if (!is_swap_pte(ptent))
		return vm_normal_page(...);

*after* the pte_none() check.

>> +	entry = pte_to_swp_entry(vmf->orig_pte);
>> +	/* Is it one of the "swap PTEs" that's not really swap? */
>> +	if (non_swap_entry(entry))
>> +		return false;
>> +
>> +	/*
>> +	 * The PTE was a true swap entry.  The page may be in the
>> +	 * swap cache.  If so, find it and return it so it may be
>> +	 * reclaimed.
>> +	 */
>> +	return lookup_swap_cache(entry, vma, addr);
> 
> If we go with handling only exclusived owned page for anon,
> I think we should apply the rule to swap cache, too.

I'm going back and forth on it.  If we're just trying to avoid causing
faults in other processes, we could add a mapcount>0 check here in
addition to the mapcount>1 checks that were added in the other patch.

But, if we want a check for true exclusivity: no other swap entries or
mappings, we need to check swap_count() too.  It's getting quite a bit
uglier as I add that it, but I guess we'll see how it looks in the end.

> Do you mind posting it as formal patch?

Yeah, I'll send something out.


  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-10 18:08 Jann Horn
2020-03-10 18:48 ` Michal Hocko
2020-03-10 19:11   ` Jann Horn
2020-03-10 21:09     ` Michal Hocko
2020-03-10 22:48       ` Dave Hansen
2020-03-11  8:45         ` Michal Hocko
2020-03-11 22:02           ` Minchan Kim
2020-03-11 23:53           ` Shakeel Butt
2020-03-12  0:18             ` Minchan Kim
2020-03-12  2:03               ` Daniel Colascione
2020-03-12 15:15                 ` Shakeel Butt
2020-03-10 20:19   ` Daniel Colascione
2020-03-10 21:40     ` Jann Horn
2020-03-10 21:52       ` Daniel Colascione
2020-03-10 22:14 ` Minchan Kim
2020-03-12  8:22 ` Michal Hocko
2020-03-12 15:40   ` Vlastimil Babka
2020-03-12 20:16   ` Minchan Kim
2020-03-12 20:26     ` Dave Hansen
2020-03-12 20:41     ` Michal Hocko
2020-03-13  2:08       ` Minchan Kim
2020-03-13  8:05         ` Michal Hocko
2020-03-13 20:59           ` Minchan Kim
2020-03-16  9:20             ` Michal Hocko
2020-03-17  1:43               ` Minchan Kim
2020-03-17  7:12                 ` Michal Hocko
2020-03-17 15:00                   ` Minchan Kim
2020-03-17 15:58                     ` Michal Hocko
2020-03-17 17:20                       ` Minchan Kim
2020-03-12 21:41     ` Dave Hansen
2020-03-13  2:00       ` Minchan Kim
2020-03-13 16:59         ` Dave Hansen [this message]
2020-03-13 21:13           ` Minchan Kim
2020-03-12 23:29     ` Jann Horn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a3a8a428-17d3-e3cb-913c-b44de12db9e4@intel.com \
    --to=dave.hansen@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=dancol@google.com \
    --cc=jannh@google.com \
    --cc=joel@joelfernandes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=minchan@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git