Re: [PATCH v14 08/39] mm/swap: Add folio_mark_accessed()

[Gregory Price <gregory.price@memverge.com>]

On Tue, Oct 10, 2023 at 07:08:15PM +0100, Matthew Wilcox wrote:
> On Sun, Oct 08, 2023 at 11:34:45AM -0400, Gregory Price wrote:
> > a) Should we update kpageflags_read to use page_folio() and get the
> >    folio flags for the head page
> > 
> > or
> > 
> > b) the above change to mark_page_accessed() should mark both the
> >    individual page flags to accessed as well as the folio head accessed.
> 
> Hi Greg, thanks for reaching out.
> 
> The referenced bit has been tracked on what is now known as a per-folio
> basis since commit 8cb38fabb6bc in 2016.  That is, if you tried to
> SetPageReferenced / ClearPageReferenced / test PageReferenced on a tail
> page, it would redirect to the head page in order to set/clear/test
> the bit in the head page's flags field.
> 
> That's not to say that all the code which sets/checks/tests this
> really understands that!  We've definitely found bugs during the folio
> work where code has mistakenly assumed that operations on a tail
> page actually affect that page rather than the whole allocation.
> 
> There's also code which is now being exposed to compound pages for the
> first time, and is not holding up well.
> 
> I hope that's helpful in finding the bug you're chasing.
> 

ah, very interesting.  So the kpageflags code doesn't appear to respect
flag locations defined in 8cb38fabb6bc

We tested this hack of a patch which swaps the page flags for the folio
head flags according to the flag locations defined in page-flags.h.

The result was that the referenced bit was almost *never* set on an
anonymous pages, no matter where we look, even when we have a program
constantly touching every page in a large chunk of memory.

Head scratcher.

Thank you for the response.  We're still very much confused, but it
looks like there are potentially many problems (unless we're
misunderstanding the reference bit).

(cc Naoya since I saw you were actively working on kpageflags for folio
 support not sure what else you're seeing).

~Gregory


diff --git a/fs/proc/page.c b/fs/proc/page.c
index 195b077c0fac..8022a41d7899 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -110,6 +110,7 @@ static inline u64 kpf_copy_bit(u64 kflags, int ubit, int kbit)
 u64 stable_page_flags(struct page *page)
 {
        u64 k;
+       u64 fh;
        u64 u;

        /*
@@ -119,7 +120,7 @@ u64 stable_page_flags(struct page *page)
        if (!page)
                return 1 << KPF_NOPAGE;

-       k = page->flags;
+       fh = *folio_flags(page_folio(folio), 0);
        u = 0;

        /*
@@ -188,20 +189,20 @@ u64 stable_page_flags(struct page *page)
                u |= 1 << KPF_SLAB;

        u |= kpf_copy_bit(k, KPF_ERROR,         PG_error);
-       u |= kpf_copy_bit(k, KPF_DIRTY,         PG_dirty);
+       u |= kpf_copy_bit(fh, KPF_DIRTY,        PG_dirty);
        u |= kpf_copy_bit(k, KPF_UPTODATE,      PG_uptodate);
        u |= kpf_copy_bit(k, KPF_WRITEBACK,     PG_writeback);

-       u |= kpf_copy_bit(k, KPF_LRU,           PG_lru);
-       u |= kpf_copy_bit(k, KPF_REFERENCED,    PG_referenced);
-       u |= kpf_copy_bit(k, KPF_ACTIVE,        PG_active);
+       u |= kpf_copy_bit(fh, KPF_LRU,          PG_lru);
+       u |= kpf_copy_bit(fh, KPF_REFERENCED,   PG_referenced);
+       u |= kpf_copy_bit(fh, KPF_ACTIVE,       PG_active);
        u |= kpf_copy_bit(k, KPF_RECLAIM,       PG_reclaim);

        if (PageSwapCache(page))
                u |= 1 << KPF_SWAPCACHE;
        u |= kpf_copy_bit(k, KPF_SWAPBACKED,    PG_swapbacked);

-       u |= kpf_copy_bit(k, KPF_UNEVICTABLE,   PG_unevictable);
+       u |= kpf_copy_bit(fh, KPF_UNEVICTABLE,  PG_unevictable);
        u |= kpf_copy_bit(k, KPF_MLOCKED,       PG_mlocked);

 #ifdef CONFIG_MEMORY_FAILURE