Date:   Wed, 9 Mar 2022 17:40:50 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Yang Shi <shy828301@...il.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Michal Hocko <mhocko@...nel.org>,
        Nadav Amit <namit@...are.com>, Rik van Riel <riel@...riel.com>,
        Roman Gushchin <guro@...com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Donald Dutile <ddutile@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>, Jan Kara <jack@...e.cz>,
        Liang Zhang <zhangliang5@...wei.com>,
        Pedro Gomes <pedrodemargomes@...il.com>,
        Oded Gabbay <oded.gabbay@...il.com>, linux-mm@...ck.org,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v1 10/15] mm/page-flags: reuse PG_slab as
 PG_anon_exclusive for PageAnon() pages

On Wed, Mar 09, 2022 at 05:57:51PM +0100, David Hildenbrand wrote:
> Hi,
> 
> On 09.03.22 16:47, Matthew Wilcox wrote:
> > On Tue, Mar 08, 2022 at 03:14:32PM +0100, David Hildenbrand wrote:
> >> The basic question we would like to have a reliable and efficient answer
> >> to is: is this anonymous page exclusive to a single process or might it
> >> be shared?
> > 
> > Is this supposed to be for PAGE_SIZE pages as well, or is it only used
> > on pages > PAGE_SIZE?
> 
> As documented, simple/ordinary PAGE_SIZE pages as well (unfortunately ,
> otherwise we'd have more space :) ).

OK.  Documentation doesn't always capture exactly what the author was
thinking, so I appreciate the clarification ;-)

> > This feels a little _too_ creative to me.  There's now an implicit
> 
> It's making the semantics of PG_slab depend on another bit in the head
> page. I agree, it's not perfect, but it's not *too* crazy. As raised in
> the cover letter, not proud of this, but I didn't really find an
> alternative for the time being.
> 
> > requirement that SL[AOU]B doesn't use the bottom two bits of
> 
> I think only the last bit (0x1)

Yeah, OK, they can use three of the four possible combinations of the
bottom two bits ;-)  Still, it's yet more constraints on use of struct
page, which aren't obvious (and are even upside down for the casual
observer).

> > I have plans to get rid of PageError and PagePrivate, but those are going
> > to be too late for you.  I don't think mappedtodisk has meaning for anon
> > pages, even if they're in the swapcache.  It would need PG_has_hwpoisoned
> 
> Are you sure it's not used if the page is in the swapcache? I have no
> detailed knowledge how file-back swap targets are handled in that
> regard. So fs experience would be highly appreciated :)

I have two arguments here.  One is based on grep:

$ git grep mappedtodisk
fs/proc/page.c: u |= kpf_copy_bit(k, KPF_MAPPEDTODISK,  PG_mappedtodisk);
include/linux/page-flags.h:     PG_mappedtodisk,        /* Has blocks allocated on-disk */
include/linux/page-flags.h:     PG_has_hwpoisoned = PG_mappedtodisk,
include/linux/page-flags.h:PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
include/trace/events/mmflags.h: {1UL << PG_mappedtodisk,        "mappedtodisk"  },              \
include/trace/events/pagemap.h: (folio_test_mappedtodisk(folio) ? PAGEMAP_MAPPEDDISK : 0) | \
mm/migrate.c:   if (folio_test_mappedtodisk(folio))
mm/migrate.c:           folio_set_mappedtodisk(newfolio);
mm/truncate.c:  folio_clear_mappedtodisk(folio);
tools/vm/page-types.c:  [KPF_MAPPEDTODISK]      = "d:mappedtodisk",

$ git grep MappedToDisk
fs/buffer.c:            SetPageMappedToDisk(page);
fs/buffer.c:    if (PageMappedToDisk(page))
fs/buffer.c:            SetPageMappedToDisk(page);
fs/ext4/readpage.c:                     SetPageMappedToDisk(page);
fs/f2fs/data.c:         SetPageMappedToDisk(page);
fs/f2fs/file.c: if (PageMappedToDisk(page))
fs/fuse/dev.c:  ClearPageMappedToDisk(newpage);
fs/mpage.c:             SetPageMappedToDisk(page);
fs/nilfs2/file.c:       if (PageMappedToDisk(page))
fs/nilfs2/file.c:                       SetPageMappedToDisk(page);
fs/nilfs2/page.c:       ClearPageMappedToDisk(page);
fs/nilfs2/page.c:               SetPageMappedToDisk(dpage);
fs/nilfs2/page.c:               ClearPageMappedToDisk(dpage);
fs/nilfs2/page.c:       if (PageMappedToDisk(src) && !PageMappedToDisk(dst))
fs/nilfs2/page.c:               SetPageMappedToDisk(dst);
fs/nilfs2/page.c:       else if (!PageMappedToDisk(src) && PageMappedToDisk(dst))
fs/nilfs2/page.c:               ClearPageMappedToDisk(dst);
fs/nilfs2/page.c:       ClearPageMappedToDisk(page);
include/linux/page-flags.h:PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)

so you can see it's _rarely_ used, and only by specific filesystems.

The swap code actually bypasses the filesystem (except for network
filesystems) and submits BIOs directly (see __swap_writepage() and
swap_readpage()).  There's no check for MappedToDisk, and nowhere to
set it/clear it.

The second argument is that MappedToDisk is used for delayed allocation
on writes for filesystems.  But swap is required to be allocated at
swapfile setup (so that the swap code can bypass the filesystem ...)
and so this flag is useless.

Of course, I may have missed something.  This is complex.

> > to shift to another bit ... but almost any bit will do for has_hwpoisoned.
> > Or have I overlooked something?
> 
> Good question, I assume we could use another bit that's not already
> defined/check on subpages of a compound page.
> 
> 
> Overloading PG_reserved would be an alternative, however, that flag can
> also indicate that the remainder of the memmap might be mostly garbage,
> so it's not that good of a fit IMHO.

I wouldn't reuse PG_reserved for anything.  Although I do have a modest
proposal that I should finish writing up ... let me start on that.

Powered by blists - more mailing lists