mm, page_alloc: Always use a captured page regardless of compaction result
diff mbox series

Message ID 20190419085133.GH18914@techsingularity.net
State New
Headers show
Series
  • mm, page_alloc: Always use a captured page regardless of compaction result
Related show

Commit Message

Mel Gorman April 19, 2019, 8:51 a.m. UTC
During the development of commit 5e1f0f098b46 ("mm, compaction: capture
a page under direct compaction"), a paranoid check was added to ensure
that if a captured page was available after compaction that it was
consistent with the final state of compaction. The intent was to catch
serious programming bugs such as using a stale page pointer and causing
corruption problems.

However, it is possible to get a captured page even if compaction was
unsuccessful if an interrupt triggered and happened to free pages in
interrupt context that got merged into a suitable high-order page. It's
highly unlikely but Li Wang did report the following warning on s390
occuring when testing OOM handling. Note that the warning is slightly
edited for clarity.

[ 1422.124060] WARNING: CPU: 0 PID: 9783 at mm/page_alloc.c:3777 __alloc_pages_direct_compact+0x182/0x190
[ 1422.124065] Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver
 nfs lockd grace fscache sunrpc pkey ghash_s390 prng xts aes_s390 des_s390
 des_generic sha512_s390 zcrypt_cex4 zcrypt vmur binfmt_misc ip_tables xfs
 libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth qdio lcs ctcm
 ccwgroup fsm dm_mirror dm_region_hash dm_log dm_mod
[ 1422.124086] CPU: 0 PID: 9783 Comm: copy.sh Kdump: loaded Not tainted 5.1.0-rc 5 #1

This patch simply removes the check entirely instead of trying to be
clever about pages freed from interrupt context. If a serious programming
error was introduced, it is highly likely to be caught by prep_new_page()
instead.

Fixes: 5e1f0f098b46 ("mm, compaction: capture a page under direct compaction")
Reported-by: Li Wang <liwang@redhat.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Vlastimil Babka April 19, 2019, 12:54 p.m. UTC | #1
On 4/19/19 10:51 AM, Mel Gorman wrote:
> During the development of commit 5e1f0f098b46 ("mm, compaction: capture
> a page under direct compaction"), a paranoid check was added to ensure
> that if a captured page was available after compaction that it was
> consistent with the final state of compaction. The intent was to catch
> serious programming bugs such as using a stale page pointer and causing
> corruption problems.
> 
> However, it is possible to get a captured page even if compaction was
> unsuccessful if an interrupt triggered and happened to free pages in
> interrupt context that got merged into a suitable high-order page. It's
> highly unlikely but Li Wang did report the following warning on s390
> occuring when testing OOM handling. Note that the warning is slightly
> edited for clarity.
> 
> [ 1422.124060] WARNING: CPU: 0 PID: 9783 at mm/page_alloc.c:3777 __alloc_pages_direct_compact+0x182/0x190
> [ 1422.124065] Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver
>  nfs lockd grace fscache sunrpc pkey ghash_s390 prng xts aes_s390 des_s390
>  des_generic sha512_s390 zcrypt_cex4 zcrypt vmur binfmt_misc ip_tables xfs
>  libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth qdio lcs ctcm
>  ccwgroup fsm dm_mirror dm_region_hash dm_log dm_mod
> [ 1422.124086] CPU: 0 PID: 9783 Comm: copy.sh Kdump: loaded Not tainted 5.1.0-rc 5 #1
> 
> This patch simply removes the check entirely instead of trying to be
> clever about pages freed from interrupt context. If a serious programming
> error was introduced, it is highly likely to be caught by prep_new_page()
> instead.
> 
> Fixes: 5e1f0f098b46 ("mm, compaction: capture a page under direct compaction")
> Reported-by: Li Wang <liwang@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Ah, noticed the new formal resend only after replying to the first one,
so here goes again:


Yup, no need for a Cc: stable on a very rare WARN_ON_ONCE. So the AI
will pick it anyway...

Acked-by: Vlastimil Babka <vbabka@suse.cz>


> ---
>  mm/page_alloc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d96ca5bc555b..cfaba3889fa2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3773,11 +3773,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	memalloc_noreclaim_restore(noreclaim_flag);
>  	psi_memstall_leave(&pflags);
>  
> -	if (*compact_result <= COMPACT_INACTIVE) {
> -		WARN_ON_ONCE(page);
> -		return NULL;
> -	}
> -
>  	/*
>  	 * At least in one zone compaction wasn't deferred or skipped, so let's
>  	 * count a compaction stall
>
Mel Gorman April 19, 2019, 1:30 p.m. UTC | #2
On Fri, Apr 19, 2019 at 02:54:54PM +0200, Vlastimil Babka wrote:
> On 4/19/19 10:51 AM, Mel Gorman wrote:
> > During the development of commit 5e1f0f098b46 ("mm, compaction: capture
> > a page under direct compaction"), a paranoid check was added to ensure
> > that if a captured page was available after compaction that it was
> > consistent with the final state of compaction. The intent was to catch
> > serious programming bugs such as using a stale page pointer and causing
> > corruption problems.
> > 
> > However, it is possible to get a captured page even if compaction was
> > unsuccessful if an interrupt triggered and happened to free pages in
> > interrupt context that got merged into a suitable high-order page. It's
> > highly unlikely but Li Wang did report the following warning on s390
> > occuring when testing OOM handling. Note that the warning is slightly
> > edited for clarity.
> > 
> > [ 1422.124060] WARNING: CPU: 0 PID: 9783 at mm/page_alloc.c:3777 __alloc_pages_direct_compact+0x182/0x190
> > [ 1422.124065] Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver
> >  nfs lockd grace fscache sunrpc pkey ghash_s390 prng xts aes_s390 des_s390
> >  des_generic sha512_s390 zcrypt_cex4 zcrypt vmur binfmt_misc ip_tables xfs
> >  libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth qdio lcs ctcm
> >  ccwgroup fsm dm_mirror dm_region_hash dm_log dm_mod
> > [ 1422.124086] CPU: 0 PID: 9783 Comm: copy.sh Kdump: loaded Not tainted 5.1.0-rc 5 #1
> > 
> > This patch simply removes the check entirely instead of trying to be
> > clever about pages freed from interrupt context. If a serious programming
> > error was introduced, it is highly likely to be caught by prep_new_page()
> > instead.
> > 
> > Fixes: 5e1f0f098b46 ("mm, compaction: capture a page under direct compaction")
> > Reported-by: Li Wang <liwang@redhat.com>
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Ah, noticed the new formal resend only after replying to the first one,
> so here goes again:
> 
> Yup, no need for a Cc: stable on a very rare WARN_ON_ONCE. So the AI
> will pick it anyway...
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 

With luck, this will be picked up and sent to Linus before 5.1 releases
and then the stable bot will not need to touch the commit at all.

Patch
diff mbox series

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d96ca5bc555b..cfaba3889fa2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3773,11 +3773,6 @@  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	memalloc_noreclaim_restore(noreclaim_flag);
 	psi_memstall_leave(&pflags);
 
-	if (*compact_result <= COMPACT_INACTIVE) {
-		WARN_ON_ONCE(page);
-		return NULL;
-	}
-
 	/*
 	 * At least in one zone compaction wasn't deferred or skipped, so let's
 	 * count a compaction stall