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

["Zi Yan" <ziy@nvidia.com>]

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@cmpxchg.org>
>> >> 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@nvidia.com>
>> >
>> > Change seems reasonable overall.
>> >
>> > Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>> >
>> >> ---
>> >>  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();


-- 
Best Regards,
Yan, Zi