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

[Kairui Song]

On Fri, Aug 28, 2026 at 12:19 AM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> Switch to double tab indentation instead while at it. Same for the other function.

Sure, will change it.

> >  {
> >       struct folio *end_folio = folio_next(folio);
> >       struct swap_cluster_info *ci = NULL;
> > @@ -3948,7 +3946,6 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
> >       bool dequeue_deferred;
> >       int ret = 0;
> >
> > -     VM_WARN_ON_ONCE(!mapping && end);
> >       /*
> >        * If this folio can be on the deferred split queue, lock out
> >        * the shrinker before freezing the ref. If the shrinker sees
> > @@ -3956,7 +3953,7 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
> >        * lock and must clean up the LRU state - the same dequeue we
> >        * will do below as part of the split.
> >        */
> > -     dequeue_deferred = folio_test_anon(folio) && old_order > 1;
> > +     dequeue_deferred = old_order > 1;
> >       if (dequeue_deferred) {
> >               struct mem_cgroup *memcg;
> >
> > @@ -3986,24 +3983,72 @@ 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 post-split folios and put them back to the right
>
> Why call it "post-split" here when it's "after-split" in the other comment?

Will just call it after-split then.

> > +      * place. Keep the head @folio frozen until the end: sub entries
> > +      * in swap cache must be updated first, so a concurrent
> > +      * swap_cache_get_folio() cannot return the head folio for a sub
> > +      * entry (folio_try_get() will fail on the head @folio until unfreeze).
> > +      */
> > +     for (new_folio = folio_next(folio); new_folio != end_folio;
> > +          new_folio = next) {
> > +             next = folio_next(new_folio);
> > +             zone_device_private_split_cb(folio, new_folio);
> > +             folio_ref_unfreeze(new_folio,
> > +                                folio_cache_ref_count(new_folio) + 1);
> > +             if (do_lru)
> > +                     lru_add_split_folio(folio, new_folio, lruvec, list);
> > +             if (ci)
> > +                     __swap_cache_replace_folio(ci, folio, new_folio);
> > +     }
>
> This smells like duplicate code now. That should better be factored out?
>

There were some discussion on this in previous series, I tried to
implement a macro or helper, but it ended up being uglier. Right now
the duplication is just about 3 lines of for loop due to the split,
and the extra loop duplication existed before, so I think it's fine at
this point, maybe a helper later if this grows.

>
> > -     zone_device_private_split_cb(folio, NULL);
> >       /*
> >        * Unfreeze @folio only after all page cache entries, which
> >        * used to point to it, have been updated with new folios.
> > @@ -4075,8 +4105,6 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
> >
> >       if (do_lru)
> >               lruvec_unlock(lruvec);
> > -     if (ci)
> > -             swap_cluster_unlock(ci);
> >
> >       return ret;
> >  }
> > @@ -4230,10 +4258,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);
> >       }
>
>
> I was briefly wondering whether shmem folios in the swapcache would now go
> through __folio_freeze_split_unmapped_anon(). But folio_check_splittable()
> rejects them.
>
> I think we should consider changing all the "if (mapping)" checks there to
> "is_anon" instead.
>
> is_anon implies no mapping and !is_anon implies that we need a mapping.

Good idea.

> This patch LGTM, but I think we should clean up __folio_split() further. Maybe
> taht happens in the next patches in this series :)
>
> Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

Thanks!