Re: [PATCH v2 4/4] mm: page_owner: use new iteration API

[Luiz Capitulino]

On 2025-02-27 08:50, David Hildenbrand wrote:
> On 25.02.25 23:30, Luiz Capitulino wrote:
> > On 2025-02-25 11:44, David Hildenbrand wrote:
> > > On 24.02.25 22:59, Luiz Capitulino wrote:
> > > > The page_ext_next() function assumes that page extension objects for a
> > > > page order allocation always reside in the same memory section, which
> > > > may not be true and could lead to crashes. Use the new page_ext
> > > > iteration API instead.
> > > >
> > > > Fixes: cf54f310d0d3 ("mm/hugetlb: use __GFP_COMP for gigantic folios")
> > > > Signed-off-by: Luiz Capitulino <luizcap@xxxxxxxxxx>
> > > > ---
> > > >    mm/page_owner.c | 61 +++++++++++++++++++++++--------------------------
> > > >    1 file changed, 29 insertions(+), 32 deletions(-)
> > >
> > > [...]
> > >
> > > >    void __reset_page_owner(struct page *page, unsigned short order)
> > > > @@ -293,11 +297,11 @@ void __reset_page_owner(struct page *page, unsigned short order)
> > > >        page_owner = get_page_owner(page_ext);
> > > >        alloc_handle = page_owner->handle;
> > > > +    page_ext_put(page_ext);
> > > >        handle = save_stack(GFP_NOWAIT | __GFP_NOWARN);
> > > > -    __update_page_owner_free_handle(page_ext, handle, order, current->pid,
> > > > +    __update_page_owner_free_handle(page, handle, order, current->pid,
> > > >                        current->tgid, free_ts_nsec);
> > > > -    page_ext_put(page_ext);
> > >
> > > I assume moving that is fine ...
> > >
> > > but I'll not that ...
> > >
> > > > -    for (i = 0; i < (1 << new_page_owner->order); i++) {
> > > > +    rcu_read_lock();
> > > > +    for_each_page_ext(&old->page, 1 << new_page_owner->order, page_ext, iter) {
> > > > +        old_page_owner = get_page_owner(page_ext);
> > > >            old_page_owner->handle = migrate_handle;
> > > > -        old_ext = page_ext_next(old_ext);
> > > > -        old_page_owner = get_page_owner(old_ext);
> > > >        }
> > > > +    rcu_read_unlock();
> > > >        page_ext_put(new_ext);
> > > >        page_ext_put(old_ext);
> > >
> > > ... here you are not moving it?
> > >
> > >
> > > In general, LGTM, only the remaining page_ext_put() are a bit confusing.
> >
> > Which part you found confusing: the fact that I'm not moving them up or that
> > we still make use of them?
>
> How we are deferring page_ext_put() when not actually working on these
> values anymore. The page_owner itself should not go away here unless we
> have a serious bug.
>
> To be precise, can't we simply do the following on top?

Yes, that looks good and I like how the new API allows for simpler code.

My only concern is that if the user is not familiar with the page_ext
internals, it might not be clear what page_ext_put() is actually
protecting in which case it looks wrong that we're using a reference
returned by get_page_owner() after releasing the lock. If you think
that that's not an issue then I can apply this change on top.

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index c9d2c688eb981..12044340adf89 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -356,26 +356,24 @@ void __split_page_owner(struct page *page, int old_order, int new_order)
>
>   void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>   {
> -       struct page_ext *old_ext;
> -       struct page_ext *new_ext;
>          struct page_ext *page_ext;
>          struct page_ext_iter iter;
>          struct page_owner *old_page_owner;
>          struct page_owner *new_page_owner;
>          depot_stack_handle_t migrate_handle;
>
> -       old_ext = page_ext_get(&old->page);
> -       if (unlikely(!old_ext))
> +       page_ext = page_ext_get(&old->page);
> +       if (unlikely(!page_ext))
>                  return;
> +       old_page_owner = get_page_owner(page_ext);
> +       page_ext_put(page_ext);
>
> -       new_ext = page_ext_get(&newfolio->page);
> -       if (unlikely(!new_ext)) {
> -               page_ext_put(old_ext);
> +       page_ext = page_ext_get(&newfolio->page);
> +       if (unlikely(!page_ext))
>                  return;
> -       }
> +       new_page_owner = get_page_owner(page_ext);
> +       page_ext_put(page_ext);
>
> -       old_page_owner = get_page_owner(old_ext);
> -       new_page_owner = get_page_owner(new_ext);
>          migrate_handle = new_page_owner->handle;
>          __update_page_owner_handle(&newfolio->page, old_page_owner->handle,
>                                     old_page_owner->order, old_page_owner->gfp_mask,
> @@ -402,9 +400,6 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>                  old_page_owner->handle = migrate_handle;
>          }
>          rcu_read_unlock();
> -
> -       page_ext_put(new_ext);
> -       page_ext_put(old_ext);
>   }
>
>   void pagetypeinfo_showmixedcount_print(struct seq_file *m,