Re: [PATCH v2 7/7] mm: switch deferred split shrinker to list_lru

David Hildenbrand (Arm)
2026-03-19T07:21:29Z

>>
>> So, in all anon alloc paths, we essentialy have
>>
>> 1) vma_alloc_folio / __folio_alloc (khugepaged being odd)
>> 2) mem_cgroup_charge / mem_cgroup_swapin_charge_folio
>> 3) memcg_list_lru_alloc_folio
>>
>> I wonder if we could do better in most cases and have something like a
>>
>> vma_alloc_anon_folio()
>>
>> That wraps the vma_alloc_folio() + memcg_list_lru_alloc_folio(), but
>> still leaves the charging to the caller?
>
> Hm, but it's the charging that figures out the memcg and sets
> folio_memcg() :(

Oh ... right. I guess we would then have to do all 3 things at the same
time, which makes the helper a bit more involved.

I'll note that collapse_file() also calls alloc_charge_folio(), but not
for allocating an anonymous folio that would have to be placed on the
deferred split queue.

>
>> The would at least combine 1) and 3) in a single API. (except for the
>> odd cases without a VMA).
>>
>> I guess we would want to skip the memcg_list_lru_alloc_folio() for
>> order-0 folios, correct?
>
> Yeah, we don't use the queue for < order-1. In deferred_split_folio():
>
> /*
> * Order 1 folios have no space for a deferred list, but we also
> * won't waste much memory by not adding them to the deferred list.
> */
> if (folio_order(folio) <= 1)
> return;
>
>>> @@ -3802,33 +3706,28 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>>> struct folio *new_folio, *next;
>>> int old_order = folio_order(folio);
>>> int ret = 0;
>>> - struct deferred_split *ds_queue;
>>> + struct list_lru_one *l;
>>>
>>> VM_WARN_ON_ONCE(!mapping && end);
>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>> - ds_queue = folio_split_queue_lock(folio);
>>> + rcu_read_lock();
>>
>> The RCU lock is for the folio_memcg(), right?
>>
>> I recall I raised in the past that some get/put-like logic (that wraps
>> the rcu_read_lock() + folio_memcg()) might make this a lot easier to get.
>>
>>
>> memcg = folio_memcg_lookup(folio)
>>
>> ... do stuff
>>
>> folio_memcg_putback(folio, memcg);
>>
>> Or sth like that.
>>
>>
>> Alternativey, you could have some helpers that do the
>> list_lru_lock+unlock etc.
>>
>> folio_memcg_list_lru_lock()
>> ...
>> folio_memcg_list_ru_unlock(l);
>>
>> Just some thoughts as inspiration :)
>
> I remember you raising this in the objcg + reparenting patches. There
> are a few more instances of
>
> rcu_read_lock()
> foo = folio_memcg()
> ...
> rcu_read_unlock()
>
> in other parts of the code not touched by these patches here, so the
> first pattern is a more universal encapsulation.
>
> Let me look into this. Would you be okay with a follow-up that covers
> the others as well?

Of course :) If list_lru lock helpers would be the right thing to do, it
might be better placed in this series.

--
Cheers,

David