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

[David Hildenbrand (Arm)]

>>
>> 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