All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Vlastimil Babka <vbabka@suse.cz>,
	David Rientjes <rientjes@google.com>,
	Hugh Dickins <hughd@google.com>, Mel Gorman <mgorman@suse.de>
Subject: Re: [PATCH] mm/compaction: avoid VM_BUG_ON(PageSlab()) in page_mapcount()
Date: Fri, 22 May 2020 18:34:21 -0700	[thread overview]
Message-ID: <20200522183421.7c52ba9650a2ad11cec2be8d@linux-foundation.org> (raw)
In-Reply-To: <158937872515.474360.5066096871639561424.stgit@buzz>

On Wed, 13 May 2020 17:05:25 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:

> Function isolate_migratepages_block() runs some checks out of lru_lock
> when choose pages for migration. After checking PageLRU() it checks extra
> page references by comparing page_count() and page_mapcount(). Between
> these two checks page could be removed from lru, freed and taken by slab.
> 
> As a result this race triggers VM_BUG_ON(PageSlab()) in page_mapcount().
> Race window is tiny. For certain workload this happens around once a year.
> 
> 
>  page:ffffea0105ca9380 count:1 mapcount:0 mapping:ffff88ff7712c180 index:0x0 compound_mapcount: 0
>  flags: 0x500000000008100(slab|head)
>  raw: 0500000000008100 dead000000000100 dead000000000200 ffff88ff7712c180
>  raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
>  page dumped because: VM_BUG_ON_PAGE(PageSlab(page))
>  ------------[ cut here ]------------
>  kernel BUG at ./include/linux/mm.h:628!
>  invalid opcode: 0000 [#1] SMP NOPTI
>  CPU: 77 PID: 504 Comm: kcompactd1 Tainted: G        W         4.19.109-27 #1
>  Hardware name: Yandex T175-N41-Y3N/MY81-EX0-Y3N, BIOS R05 06/20/2019
>  RIP: 0010:isolate_migratepages_block+0x986/0x9b0
> 
> 
> To fix just opencode page_mapcount() in racy check for 0-order case and
> recheck carefully under lru_lock when page cannot escape from lru.
> 
> Also add checking extra references for file pages and swap cache.

I dunno, this code looks quite nasty.  I'm more thinking we should
revert and rethink David's 119d6d59dcc0980dcd58 ("mm, compaction: avoid
isolating pinned pages").

> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -935,12 +935,16 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		}
>  
>  		/*
> -		 * Migration will fail if an anonymous page is pinned in memory,
> +		 * Migration will fail if an page is pinned in memory,
>  		 * so avoid taking lru_lock and isolating it unnecessarily in an
> -		 * admittedly racy check.
> +		 * admittedly racy check simplest case for 0-order pages.
> +		 *
> +		 * Open code page_mapcount() to avoid VM_BUG_ON(PageSlab(page)).
> +		 * Page could have extra reference from mapping or swap cache.
>  		 */
> -		if (!page_mapping(page) &&
> -		    page_count(page) > page_mapcount(page))
> +		if (!PageCompound(page) &&
> +		    page_count(page) > atomic_read(&page->_mapcount) + 1 +
> +				(!PageAnon(page) || PageSwapCache(page)))
>  			goto isolate_fail;

OK, we happened to notice this because we happened to trigger a
!PageSlab assertion.  But if this page has been freed and reused for
slab, it could have been reused for *anything*?  Perhaps it was reused
as a migratable page which we'll go ahead and migrate even though we no
longer should.  There are various whacky parts of the kernel which
(ab)use surprising struct page fields in surprising ways - how do we
know it isn't one of those which now happens to look like a migratable
page?

I also worry about the next test:

		/*
		 * Only allow to migrate anonymous pages in GFP_NOFS context
		 * because those do not depend on fs locks.
		 */
		if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page))
			goto isolate_fail;

This page isn't PageLocked(), is it?  It could be a recycled page which
is will be getting its ->mapping set one nanosecond hence.


>  		/*
> @@ -975,6 +979,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				low_pfn += compound_nr(page) - 1;
>  				goto isolate_fail;
>  			}
> +
> +			/* Recheck page extra references under lock */
> +			if (page_count(page) > page_mapcount(page) +
> +				    (!PageAnon(page) || PageSwapCache(page)))
> +				goto isolate_fail;
>  		}
>  
>  		lruvec = mem_cgroup_page_lruvec(page, pgdat);
> 

  parent reply	other threads:[~2020-05-23  1:34 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 14:05 [PATCH] mm/compaction: avoid VM_BUG_ON(PageSlab()) in page_mapcount() Konstantin Khlebnikov
2020-05-13 18:32 ` Andrew Morton
2020-05-13 19:28   ` Konstantin Khlebnikov
2020-05-13 21:35     ` Vlastimil Babka
2020-05-23  1:34 ` Andrew Morton [this message]
2020-05-23 13:55   ` Konstantin Khlebnikov
2020-05-23 13:55     ` Konstantin Khlebnikov
2020-05-24  1:01 ` Hugh Dickins
2020-05-24  1:01   ` Hugh Dickins
2020-05-24 11:21   ` Konstantin Khlebnikov
2020-05-24 19:12     ` Hugh Dickins
2020-05-24 19:12       ` Hugh Dickins
2020-06-02  4:05   ` Hugh Dickins
2020-06-02  4:05     ` Hugh Dickins
2020-06-02  4:13     ` Andrew Morton
2020-06-02 11:28     ` Alex Shi

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=20200522183421.7c52ba9650a2ad11cec2be8d@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=khlebnikov@yandex-team.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=rientjes@google.com \
    --cc=vbabka@suse.cz \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.