Re: [PATCH v2 1/2] mm/huge_memory: use folio's memcg inside __folio_split()

[Lorenzo Stoakes (ARM)]

On Mon, Aug 03, 2026 at 11:23:39AM -0400, Zi Yan wrote:
> On Mon Aug 3, 2026 at 11:07 AM EDT, Lorenzo Stoakes (ARM) wrote:
> > On Mon, Aug 03, 2026 at 11:02:30AM -0400, Zi Yan wrote:
> >> On Sat Aug 1, 2026 at 5:36 AM EDT, Lorenzo Stoakes (ARM) wrote:
> >> > On Thu, Jul 30, 2026 at 10:18:00PM -0400, Zi Yan wrote:
> >> >> During a pagecache folio split, an xarray node allocation can happen and
> >> >> needs to charge at folio's memcg instead of folio split invoker's memcg,
> >> >> because for example folio split can happen during reclaim and reclaim's
> >> >> active memcg might not be folio's memcg. Switch to folio's memcg at the
> >> >> beginning and switch back afterwards.
> >> >
> >> > I assume this is the only allocation? I guess in general it makes sense to have
> >> > the folio's memcg be active here regardless.
> >> >
> >> >>
> >> >> Suggested-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> >> >> Fixes: 6b24ca4a1a8d4 ("mm: Use multi-index entries in the page cache")
> >> >
> >> > Cc: stable?
> >>
> >> Like you said above, only xas_split_alloc() is affected. And we have not
> >> seen related workingset regression report (like what Johannes reported
> >> in commit 7b785645e8f13 ("mm: fix page cache convergence regression")).
> >> It might be OK to not backport.
> >>
> >> Johannes, what is your take on this?
> >>
> >> >
> >> >> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
> >> >
> >> > Change seems reasonable overall.
> >> >
> >> > Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> >> >
> >> >> ---
> >> >>  mm/huge_memory.c | 20 ++++++++++++++++----
> >> >>  1 file changed, 16 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> >> index 04e8a6b553435..b9c2d8908e564 100644
> >> >> --- a/mm/huge_memory.c
> >> >> +++ b/mm/huge_memory.c
> >> >> @@ -4063,34 +4063,42 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >> >>  	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
> >> >>  	struct folio *end_folio = folio_next(folio);
> >> >>  	bool is_anon = folio_test_anon(folio);
> >> >> +	struct mem_cgroup *memcg, *old_memcg;
> >> >>  	struct address_space *mapping = NULL;
> >> >>  	struct anon_vma *anon_vma = NULL;
> >> >>  	int old_order = folio_order(folio);
> >> >>  	struct folio *new_folio, *next;
> >> >>  	int nr_shmem_dropped = 0;
> >> >>  	enum ttu_flags ttu_flags = 0;
> >> >> -	int ret;
> >> >>  	pgoff_t end = 0;
> >> >> +	int ret;
> >> >>
> >> >>  	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> >> >>  	VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
> >> >>
> >> >>  	if (folio != page_folio(split_at) || folio != page_folio(lock_at)) {
> >> >>  		ret = -EINVAL;
> >> >> -		goto out;
> >> >> +		goto out_no_memcg;
> >> >>  	}
> >> >>
> >> >>  	if (new_order >= old_order) {
> >> >>  		ret = -EINVAL;
> >> >> -		goto out;
> >> >> +		goto out_no_memcg;
> >> >>  	}
> >> >>
> >> >>  	ret = folio_check_splittable(folio, new_order, split_type);
> >> >>  	if (ret) {
> >> >>  		VM_WARN_ONCE(ret == -EINVAL, "Tried to split an unsplittable folio");
> >> >> -		goto out;
> >> >> +		goto out_no_memcg;
> >> >
> >> > This function really badly needs splitting up and probably some cleanup.h work :)
> >>
> >> You mean folio_check_splittable()? You want to move -EINVAL checks a
> >> separate one?
> >
> > No __folio_split().
> >
> > Comment about cleanup.h really was the whole pattern of goto xxx for various
> > levels of unwinding things.
> >
> > But really I mean the folio splitting code in general, there's a lot of
> > massive-complicated-functions with a million things going on at once,
> > __folio_freeze_and_split_unmapped() is another.
> >
> > Feels like we should really have this stuff in something like mm/folio.c anyway
> > too now that's renamed :)
> >
>
> I agree that __folio_split() is handling multiple cases, anon, shmem,
> pagecache, all together. Do you prefer:
>
> 1. split __folio_split() to handle each case in a separate function with
> some code duplication, like xarray for pagecache and shmem,
> freeze/unfreeze folio for all;
>
> or
>
> 2. encapulate per-case code in small functions, like
> if (is_anon)
>     split_prepare_anon();
> else
>     split_prepare_file_backed();
>
> __folio_freeze_and_split_unmapped();
>
> if (is_anon)
>     post_split_anon();
> else
>     post_split_file_backed();

Well these 'post' functions are a bit confusing so I guess I'd say experiment
with different approaches and see which ones end up with the nicest code :)

>
>
> --
> Best Regards,
> Yan, Zi
>

--
Cheers, Lorenzo