Re: [PATCH RFC 3/3] mm: remove folio_test_anon(folio)==false path in __folio_add_anon_rmap()

[Barry Song <21cnbao@gmail.com>]

On Fri, Jun 14, 2024 at 11:10 PM David Hildenbrand <david@redhat.com> wrote:
>
> >> I don't think that is required? We are only working with anon folios. Or
> >> were you able to trigger this? (which would be weird)
> >
> > I didn't trigger this. but I am not sure if kfifo is always anon based on
> > the code context.
> >
> > for page,  it is 100% anon(otherwise "goto out"), but I am not quite
> > sure about kpage
> > by the code context.
> >
> > static int try_to_merge_one_page(struct vm_area_struct *vma,
> >                                   struct page *page, struct page *kpage)
> > {
> >          pte_t orig_pte = __pte(0);
> >          int err = -EFAULT;
> >
> >          if (page == kpage)                      /* ksm page forked */
> >                  return 0;
> >
> >          if (!PageAnon(page))
> >                  goto out;
> >          ....
> > }
> >
> > Then I saw this
> >
> > static int replace_page(struct vm_area_struct *vma, struct page *page,
> >                          struct page *kpage, pte_t orig_pte)
> > {
> >          ...
> >          VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
> >          VM_BUG_ON_FOLIO(folio_test_anon(kfolio) && PageAnonExclusive(kpage),
> >                          kfolio);
> > }
> >
> > If kfolio is always anon, we should have used
> > VM_BUG_ON_FOLIO(PageAnonExclusive(kpage), folio)
> > just like
> > VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
> > without "folio_test_anon(kfolio)".
> >
> > So I lost my way.
>
> try_to_merge_one_page() is either called with a KSM page (anon) from
> try_to_merge_with_ksm_page() or with the shared zeropage (!anon and must
> never become anon) from cmp_and_merge_page().
>
> So in replace_page(), we either have an ksm/anon page or the shared
> zeropage.
>
> We never updated the documentation of replace_page() to spell out that
> "kpage" can also be the shared zeropage.
>
> Note how replace_page() calls folio_add_anon_rmap_pte() not for the
> shared zeropage.
>
> If we would have to craft a new anon page things would be going terribly
> wrong.
>
> So not, this (!anon -> anon) must not happen and if it were to happen,
> it would be a real bug and your check in  folio_add_anon_rmap_pte()
> would catch it.

Thanks very much for the explanation. I wonder if the below can help
improve the doc. If yes, I may add a separate patch for it in v2.

diff --git a/mm/ksm.c b/mm/ksm.c
index d2641bc2efc9..56b10265e617 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1411,14 +1411,13 @@ static int replace_page(struct vm_area_struct
*vma, struct page *page,
                goto out_mn;
        }
        VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
-       VM_BUG_ON_FOLIO(folio_test_anon(kfolio) && PageAnonExclusive(kpage),
-                       kfolio);
-
        /*
         * No need to check ksm_use_zero_pages here: we can only have a
         * zero_page here if ksm_use_zero_pages was enabled already.
         */
        if (!is_zero_pfn(page_to_pfn(kpage))) {
+               VM_BUG_ON_FOLIO(!folio_test_anon(kfolio) ||
PageAnonExclusive(kpage),
+                       kfolio);
                folio_get(kfolio);
                folio_add_anon_rmap_pte(kfolio, kpage, vma, addr, RMAP_NONE);
                newpte = mk_pte(kpage, vma->vm_page_prot);

>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry