Re: [PATCH -next 3/5] mm/mglru: extend shrink_one for both lrugen and non-lrugen

[Chen Ridong <chenridong@huaweicloud.com>]

On 2025/12/23 5:36, Shakeel Butt wrote:
> On Tue, Dec 16, 2025 at 09:14:45AM +0800, Chen Ridong wrote:
>>
>>
>> On 2025/12/16 5:13, Johannes Weiner wrote:
>>> On Tue, Dec 09, 2025 at 01:25:55AM +0000, Chen Ridong wrote:
>>>> From: Chen Ridong <chenridong@huawei.com>
>>>>
>>>> Currently, flush_reclaim_state is placed differently between
>>>> shrink_node_memcgs and shrink_many. shrink_many (only used for gen-LRU)
>>>> calls it after each lruvec is shrunk, while shrink_node_memcgs calls it
>>>> only after all lruvecs have been shrunk.
>>>>
>>>> This patch moves flush_reclaim_state into shrink_node_memcgs and calls it
>>>> after each lruvec. This unifies the behavior and is reasonable because:
>>>>
>>>> 1. flush_reclaim_state adds current->reclaim_state->reclaimed to
>>>>    sc->nr_reclaimed.
>>>> 2. For non-MGLRU root reclaim, this can help stop the iteration earlier
>>>>    when nr_to_reclaim is reached.
>>>> 3. For non-root reclaim, the effect is negligible since flush_reclaim_state
>>>>    does nothing in that case.
>>>>
>>>> After moving flush_reclaim_state into shrink_node_memcgs, shrink_one can be
>>>> extended to support both lrugen and non-lrugen paths. It will call
>>>> try_to_shrink_lruvec for lrugen root reclaim and shrink_lruvec otherwise.
>>>>
>>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>>> ---
>>>>  mm/vmscan.c | 57 +++++++++++++++++++++--------------------------------
>>>>  1 file changed, 23 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index 584f41eb4c14..795f5ebd9341 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -4758,23 +4758,7 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>>>>  	return nr_to_scan < 0;
>>>>  }
>>>>  
>>>> -static void shrink_one(struct lruvec *lruvec, struct scan_control *sc)
>>>> -{
>>>> -	unsigned long scanned = sc->nr_scanned;
>>>> -	unsigned long reclaimed = sc->nr_reclaimed;
>>>> -	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>> -	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>>> -
>>>> -	try_to_shrink_lruvec(lruvec, sc);
>>>> -
>>>> -	shrink_slab(sc->gfp_mask, pgdat->node_id, memcg, sc->priority);
>>>> -
>>>> -	if (!sc->proactive)
>>>> -		vmpressure(sc->gfp_mask, memcg, false, sc->nr_scanned - scanned,
>>>> -			   sc->nr_reclaimed - reclaimed);
>>>> -
>>>> -	flush_reclaim_state(sc);
>>>> -}
>>>> +static void shrink_one(struct lruvec *lruvec, struct scan_control *sc);
>>>>  
>>>>  static void shrink_many(struct pglist_data *pgdat, struct scan_control *sc)
>>>>  {
>>>> @@ -5760,6 +5744,27 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
>>>>  	return inactive_lru_pages > pages_for_compaction;
>>>>  }
>>>>  
>>>> +static void shrink_one(struct lruvec *lruvec, struct scan_control *sc)
>>>> +{
>>>> +	unsigned long scanned = sc->nr_scanned;
>>>> +	unsigned long reclaimed = sc->nr_reclaimed;
>>>> +	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>> +	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>>> +
>>>> +	if (lru_gen_enabled() && root_reclaim(sc))
>>>> +		try_to_shrink_lruvec(lruvec, sc);
>>>> +	else
>>>> +		shrink_lruvec(lruvec, sc);
>>>
>>
>> Hi Johannes, thank you for your reply.
>>
>>> Yikes. So we end up with:
>>>
>>> shrink_node_memcgs()
>>>   shrink_one()
>>>     if lru_gen_enabled && root_reclaim(sc)
>>>       try_to_shrink_lruvec(lruvec, sc)
>>>     else
>>>       shrink_lruvec()
>>>         if lru_gen_enabled && !root_reclaim(sc)
>>>           lru_gen_shrink_lruvec(lruvec, sc)
>>>             try_to_shrink_lruvec()
>>>
>>> I think it's doing too much at once. Can you get it into the following
>>> shape:
>>>
>>
>> You're absolutely right. This refactoring is indeed what patch 5/5 implements.
>>
>> With patch 5/5 applied, the flow becomes:
>>
>> shrink_node_memcgs()
>>     shrink_one()
>>         if lru_gen_enabled
>> 	    lru_gen_shrink_lruvec  --> symmetric with else shrink_lruvec()
>> 		if (root_reclaim(sc))  --> handle root reclaim.
>> 		    try_to_shrink_lruvec()
>> 		else
>> 		    ...
>> 		    try_to_shrink_lruvec()
>> 	else
>> 	    shrink_lruvec()
>>
>> This matches the structure you described.
>>
>> One note: shrink_one() is also called from lru_gen_shrink_node() when memcg is disabled, so I
>> believe it makes sense to keep this helper.
> 
> I think we don't need shrink_one as it can be inlined to its callers and
> also shrink_node_memcgs() already handles mem_cgroup_disabled() case, so
> lru_gen_shrink_node() should not need shrink_one for such case.
> 
I think you mean:

shrink_node
    lru_gen_shrink_node
    // We do not need to handle memcg-disabled case here,
    // because shrink_node_memcgs can already handle it.
	shrink_node_memcgs
	    for each memcg:
	        if lru_gen_enabled:
		    lru_gen_shrink_lruvec()
		else
		    shrink_lruvec()
	    shrink_slab(sc->gfp_mask, pgdat->node_id, memcg, sc->priority);
	    if (!sc->proactive)
		vmpressure(...)
	    flush_reclaim_state(sc);

With this structure, both shrink_many and shrink_one are no longer needed. That looks much cleaner.
I will update it accordingly.

Thank you very much.

>>
>>> shrink_node_memcgs()
>>>   for each memcg:
>>>     if lru_gen_enabled:
>>>       lru_gen_shrink_lruvec()
>>>     else
>>>       shrink_lruvec()
>>>
> 
> I actually like what Johannes has requested above but if that is not
> possible without changing some behavior then let's aim to do as much as
> possible in this series while keeping the same behavior. In a followup
> we can try to combine the behavior part.
> 
>>
>> Regarding the patch split, I currently kept patch 3/5 and 5/5 separate to make the changes clearer
>> in each step. Would you prefer that I merge patch 3/5 with patch 5/5, so the full refactoring
>> appears in one patch?
>>
>> Looking forward to your guidance.

-- 
Best regards,
Ridong