Re: [PATCH v3] mm/page_alloc: clear page->private in free_pages_prepare()

[David Hildenbrand (Arm)]

> > I recall that freeing pages with page->private set was allowed. Although
> > I once wondered whether we should actually change that.
>
> But if that is allowed, we can end up with tail page's private non zero,
> because that free page can merge with a lower PFN buddy and its ->private
> is not reset. See __free_one_page().

Right. Or someone could use page->private on tail pages and free non-zero ->private that way.

[...]

> For the issue reported by Mikhail[2], the page comes from vmalloc(), so it will not be split.
> For other cases, a page/folio needs to be compound to be splittable and prep_compound_tail()
> sets all tail page's private to 0. So that check is not that useful.

Thanks.

> And the issue we are handling here is non compound high order page allocation. No one is
> clearing ->private for all pages right now.

Right.

> OK, I think we want to decide whether it is OK to have a page with set ->private at
> page free time.

Right. And whether it is okay to have any tail->private be non-zero.

> If no, we can get this patch in and add a VM_WARN_ON_ONCE(page->private)
> to catch all violators. If yes, we can use Mikhail's original patch, zeroing ->private
> in split_page() and add a comment on ->private:
>
> 1. for compound page allocation, prep_compound_tail() is responsible for resetting ->private;
> 2. for non compound high order page allocation, split_page() is responsible for resetting ->private.

Ideally, I guess, we would minimize the clearing of the ->private fields.

If we could guarantee that *any* pages in the buddy have ->private clear, maybe
prep_compound_tail() could stop clearing it (and check instead).

So similar to what Vlasta said, maybe we want to (not check but actually clear):


diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e4104973e22f..4960a36145fe 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1410,6 +1410,7 @@ __always_inline bool free_pages_prepare(struct page *page,
                                }
                        }
                        (page + i)->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
+                       set_page_private(page + i, 0);
                }
        }
        if (folio_test_anon(folio)) {


And the try removing any other unnecessary clearing (like in prep_compound_tail()).


--
Cheers,

David