Re: [PATCH] mm: fix long time stall from mm_populate

[Minchan Kim]

On Wed, Feb 12, 2020 at 10:28:51AM -0800, Matthew Wilcox wrote:
> On Wed, Feb 12, 2020 at 09:40:15AM -0800, Minchan Kim wrote:
> > How about this?
> > 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 1bf83c8fcaa7..d07d602476df 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -363,8 +363,28 @@ PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
> >  /* PG_readahead is only used for reads; PG_reclaim is only for writes */
> >  PAGEFLAG(Reclaim, reclaim, PF_NO_TAIL)
> >  	TESTCLEARFLAG(Reclaim, reclaim, PF_NO_TAIL)
> > -PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> > -	TESTCLEARFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> > +
> > +SETPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> > +CLEARPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> > +
> > +/*
> > + * Since PG_readahead is shared with PG_reclaim of the page flags,
> > + * PageReadahead should double check whether it's readahead marker
> > + * or PG_reclaim. It could be done by PageWriteback check because
> > + * PG_reclaim is always with PG_writeback.
> > + */
> > +static inline int PageReadahead(struct page *page)
> > +{
> > +	VM_BUG_ON_PGFLAGS(PageCompound(page), page);
> > +	return test_bit(PG_reclaim, &(page)->flags) && !PageWriteback(page);
> 
> Why not ...
> 
> 	return page->flags & (1UL << PG_reclaim | 1UL << PG_writeback) ==
> 		(1UL << PG_reclaim);
> 
> > +static inline int TestClearPageReadahead(struct page *page)
> > +{
> > +	VM_BUG_ON_PGFLAGS(PageCompound(page), page);
> > +
> > +	return test_and_clear_bit(PG_reclaim, &page->flags) && !PageWriteback(page);
> 
> That's definitely wrong.  It'll clear PageReclaim and then pretend it did
> nothing wrong.
> 
> 	return !PageWriteback(page) ||
> 		test_and_clear_bit(PG_reclaim, &page->flags);
> 

Much better, Thanks for the review, Matthew!
If there is no objection, I will send two patches to Andrew.
One is PageReadahead strict, the other is limit retry from mm_populate.