Re: [PATCH 1/2] mm: Use owner_priv bit for PageSwapCache, valid when PageSwapBacked

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, 3 Nov 2016 19:20:23 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> On Wed, 2 Nov 2016, Nicholas Piggin wrote:
> 
> > Can we do this? Seems too easy, I must be missing something.
> 
> I have not tried running with it, which might show up a few more
> adjustments, but no showstoppers: I think you can well do this.

Hi Hugh,

Great, thanks for taking a look.

> I'll be sorry to see "owner_priv_1" in my pageflags output,
> but oh well.

We might be able to complicate that with some logic to
derive the fact it's swap cache.

> The only thing I think you miss is PAGE_FLAGS_CHECK_AT_FREE:
> that includes PG_swapcache, but not PG_owner_priv_1 or its
> synonyms PG_checked, PG_pinned, PG_foreign (I wish we didn't
> do those PG_synonyms!) - so, we've been making sure that a page
> is never freed with the swapcache bit set, but nobody has cared
> about the others, so you might hit somewhere they're not cleared
> before freeing a page.
> 
> Easily remedied by removing PG_swapcache from the list,
> I don't remember that PG_swapcache check ever being important.

Okay I'll have to take a look at that. I think the kernel test
robot might have thrown up a bad page flag for this, but I
haven't got around to looking at it yet. I'll get to it next
week.

> 
> shmem_replace_page() looks wrong to me currently (where do
> PageLocked and PageSwapBacked get set?), but that's just
> something noticed in considering your patch (it's reassuring
> if SwapBacked is always set before SwapCache), not a problem
> with your patch.
> 
> When mucking with pageflags, two easily overlooked places worth
> checking are mm/huge_memory.c __split_huge_page_tail() (no
> problem there) and mm/migrate.c migrate_page_move_mapping()
> and migrate_page_copy() (look okay, though some repetition).

Okay, I'll go over the patch again with your comments in mind.


> > ---
> >  include/linux/page-flags.h     | 12 ++++++++++--
> >  include/trace/events/mmflags.h |  1 -
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 74e4dda..58d30b8 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -87,7 +87,6 @@ enum pageflags {
> >  	PG_private_2,		/* If pagecache, has fs aux data */
> >  	PG_writeback,		/* Page is under writeback */
> >  	PG_head,		/* A head page */
> > -	PG_swapcache,		/* Swap page: swp_entry_t in private */
> >  	PG_mappedtodisk,	/* Has blocks allocated on-disk */
> 
> Did you consider using PG_mappedtodisk instead of PG_owner_priv_1?
> I think it's an even better candidate, actually having a relevant
> name, and also only used on file pages before now, I believe.
> 
> But you may have found a good reason not to use it, or at least
> enough doubt to avoid it (tmpfs does not call cleancache_init_fs():
> I believe that's enough to make __delete_from_page_cache() safe).

Well... PG_mappedtodisk is *almost* another PG_owner_priv_. We probably
can't get rid of it because some users use PG_checked as well. So the
similarity of semantics is certainly there, but I wouldn't really like
to see them start sharing codepaths which would turn it from a private
flag into an mm-wide flag.

> 
> >  	PG_reclaim,		/* To be reclaimed asap */
> >  	PG_swapbacked,		/* Page is backed by RAM/swap */
> > @@ -110,6 +109,9 @@ enum pageflags {
> >  	/* Filesystems */
> >  	PG_checked = PG_owner_priv_1,
> >  
> > +	/* SwapBacked */
> > +	PG_swapcache = PG_owner_priv_1,	/* Swap page: swp_entry_t in private */
> > +
> >  	/* Two page bits are conscripted by FS-Cache to maintain local caching
> >  	 * state.  These bits are set on pages belonging to the netfs's inodes
> >  	 * when those inodes are being locally cached.
> > @@ -314,7 +316,13 @@ PAGEFLAG_FALSE(HighMem)
> >  #endif
> >  
> >  #ifdef CONFIG_SWAP
> > -PAGEFLAG(SwapCache, swapcache, PF_NO_COMPOUND)
> > +static __always_inline int PageSwapCache(struct page *page)
> > +{
> > +	return PageSwapBacked(page) && test_bit(PG_swapcache, &page->flags);
> > +
> > +}
> > +SETPAGEFLAG(SwapCache, swapcache, PF_NO_COMPOUND)
> > +CLEARPAGEFLAG(SwapCache, swapcache, PF_NO_COMPOUND)
> 
> The people interested in swapping THPs might want to be warned of that,
> let's Cc them in case.
> 
> Don't expect any comment from me on the meatier 2/2 patch to
> unlock_page(): I'll leave that to you and Linus and Peter.

Thanks,
Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>