Re: [PATCH RFC 04/13] mm/huge_memory: split the routine for splitting anon and file folio

[Kairui Song]

On Sun, Aug 9, 2026 at 2:52 AM Zi Yan <ziy@xxxxxxxxxx> wrote:
>
> On Fri Aug 7, 2026 at 5:17 PM EDT, Kairui Song via B4 Relay wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > No functional change intended. Before adding more logic, split
> > __folio_freeze_and_split_unmapped() into an anon and a file variant so
> > each path can evolve independently. The two paths shared little beyond
> > the folio freeze call, the LRU locking, and the unfreeze skeleton, but
> > differed in all other per-folio bookkeeping and routines.
>
> While at it, can you rename __split_unmapped_folio() to
> __split_frozen_folio() to reflect the actual folio state? It is causing
> confusion and people tried to use __split_unmapped_folio() on non frozen
> folios.

Will do.

> > @@ -3987,24 +3984,73 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
> >               rcu_read_unlock();
> >       }
> >
> > -     if (mapping) {
> > +     if (folio_test_swapcache(folio))
> > +             ci = swap_cluster_get_and_lock(folio);
> > +
> > +     if (do_lru)
> > +             lruvec = folio_lruvec_lock(folio);
> > +
> > +     ret = __split_unmapped_folio(folio, new_order, split_at, NULL,
> > +                                  NULL, split_type);
> > +
> > +     /*
> > +      * Unfreeze the after-split folios and put them back to the right
> > +      * place, keeping the head @folio frozen until the end. While the
> > +      * folio is in the swap cache, the sub entries must be updated with
> > +      * their after-split folios before the head is unfrozen, so a
> > +      * concurrent swap_cache_get_folio() cannot return the head folio
> > +      * for a sub entry. Keeping the head frozen throughout also stops a
> > +      * parallel folio_try_get() from observing a partially split folio.
> > +      */
> > +     for (new_folio = folio_next(folio); new_folio != end_folio;
> > +          new_folio = folio_next(new_folio)) {
>
> Please keep the existing for loop pattern by using next =
> folio_next(new_folio) in the loop buddy.
>
> Hugh pointed out an issue when I did the above for loop pattern[1].
> Basically, folio_next() reads folio_nr_pages() and relies on a stable
> new_folio input. In my old code, the input of folio_next() can be freed
> and causing oops. In your code, that does not apply, but it can bite
> people in the future the loop body changes and new_folio's lifetime ends
> before the for loop finishes.
>
> Maybe add a comment to explain why next = folio_next(new_folio) should
> be used.

Sure, I'll try if a macro can be used to deduplicate it.

>
> [1] https://lore.kernel.org/all/2fae27fe-6e2e-3587-4b68-072118d80cf8@xxxxxxxxxx/
>

...

> > @@ -4231,10 +4259,14 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >                       ret = -EAGAIN;
> >                       goto fail;
> >               }
> > +             ret = __folio_freeze_split_unmapped_file(folio, new_order, split_at, &xas, mapping,
> > +                                                      true, list, split_type, end,
> > +                                                      &nr_shmem_dropped);
> > +     } else {
> > +             ret = __folio_freeze_split_unmapped_anon(folio, new_order, split_at, true,
> > +                                                      list, split_type);
> >       }
> >
> > -     ret = __folio_freeze_and_split_unmapped(folio, new_order, split_at, &xas, mapping,
> > -                                             true, list, split_type, end, &nr_shmem_dropped);
> >  fail:
> >       if (mapping)
> >               xas_unlock(&xas);
> > @@ -4334,9 +4366,8 @@ int folio_split_unmapped(struct folio *folio, unsigned int new_order)
> >               return -EAGAIN;
> >
> >       local_irq_disable();
> > -     ret = __folio_freeze_and_split_unmapped(folio, new_order, &folio->page, NULL,
> > -                                             NULL, false, NULL, SPLIT_TYPE_UNIFORM,
> > -                                             0, NULL);
> > +     ret = __folio_freeze_split_unmapped_anon(folio, new_order, &folio->page,
> > +                                              false, NULL, SPLIT_TYPE_UNIFORM);
> >       local_irq_enable();
> >       return ret;
> >  }
>
> There are some code duplications but overall looks good to me. The lru
> lock, unfreeze loop, and the last unfreeze are replicated across two
> functions. I cannot think of an easy alternative. A tiny improvement
> might be instead of replicating unfreeze comments, changing one to point
> to the other one and asking the code should be in sync.

Good suggestion, thanks! I think code duplications could be further
reduced by a few macros or helpers. In following patches, removing
many if branches and streamlining the workflow actually improved it as
a whole, so I think it's worth it.