On Thu, 3 Nov 2016 19:20:23 -0700 (PDT)
Hugh Dickins <hughd@xxxxxxxxxx> 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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>