| Message ID | 20190419085133.GH18914@techsingularity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, page_alloc: Always use a captured page regardless of compaction result | expand |
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 >
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.
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
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(-)