Re: [PATCH v4 2/5] mm/vmscan: extract folio_free() from shrink_folio_list()

[David Hildenbrand (Arm)]

On 6/17/26 14:17, David Hildenbrand (Arm) wrote:
> On 5/25/26 16:57, Zhang Peng wrote:
>> shrink_folio_list() contains a self-contained folio-freeing section:
>> buffer release, lazyfree, __remove_mapping, and folio_batch drain.
>> Extract it into folio_free() to reduce the size of shrink_folio_list()
>> and make the freeing step independently readable.
>>
>> No functional change.
> 
> As you are touching the code, I'll suggest some simple improvements as part of
> the move.
> 
>>
>> Signed-off-by: Zhang Peng <bruzzhang@xxxxxxxxxxx>
>> ---
>>  mm/vmscan.c | 168 +++++++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 92 insertions(+), 76 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 886d8b4843aa..b31f67801836 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1072,6 +1072,95 @@ static void folio_activate_locked(struct folio *folio,
>>  	}
>>  }
>>  
>> +static bool folio_free(struct folio *folio, struct folio_batch *free_folios,
> 
> I don't quite like the function name, as it sounds like something very generic,
> when it's really specific to memory reclaim code and does things like removing
> the folio from the pagecache.
> 
> Any way we can easily make that clearer? I think it should contain something
> about release and reclaim.
> 
> folio_release_for_reclaim() or just "folio_reclaim()" ? Not sure.
> 
> 
> 
>> +		struct scan_control *sc, struct reclaim_stat *stat,
>> +		unsigned int *nr_reclaimed)
> 
> Instead of returning a bool, can we just return 0 / -EBUSY? IMHO, a bool mostly
> only makes sense if the function name implies some sort of yes/no test. (there
> are exceptions).

Stumbling over patch #4, I think if we call this something with a "try", a bool
would be fine.

-- 
Cheers,

David