Re: [PATCH v1] mm: unqueue deferred split folios outside the lruvec lock

[David Hildenbrand (Arm)]

On 10/14/26 00:13, Joanne Koong wrote:
> move_folios_to_lru(), folio_batch_move_lru() and folios_put_refs() call
> folio_unqueue_deferred_split() on folios where the refcount has dropped
> to zero. In these paths, folio_unqueue_deferred_split() is called with
> the lruvec lock often held (move_folios_to_lru() always holds it and
> folio_batch_move_lru()/folios_put_refs() usually hold it for every folio
> in the batch except the first). However, it's not needed to hold the
> lruvec lock.
> 
> The lruvec lock protects folio->lru and the reclaim LRU lists whereas
> the deferred split queue lives on folio->_deferred_list in
> deferred_split_lru. The deferred_split_lru is a list_lru with its own
> per-memcg per-node lock.
> 
> There are already other existing callers that call
> folio_unqueue_deferred_split() without holding the lruvec lock (eg
> __folio_put(), shrink_folio_list(), __folio_migrate_mapping(), ...).
> 
> Add folio_batch_unqueue_deferred_split() and use it in
> move_folios_to_lru(), folio_batch_move_lru(), and folios_put_refs() to
> unqueue each batch of zero-refcount folios from the deferred split list
> after the lruvec lock is released and before
> mem_cgroup_uncharge_folios(). This helps shorten the lruvec lock's
> critical sections, which run with IRQs disabled and are highly
> contended.
> 
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
>  mm/folio.c    | 24 +++++++++++++++++++-----
>  mm/internal.h |  1 +
>  mm/vmscan.c   |  3 ++-
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/folio.c b/mm/folio.c
> index 35e242b48870..85144078e635 100644
> --- a/mm/folio.c
> +++ b/mm/folio.c
> @@ -109,6 +109,19 @@ EXPORT_SYMBOL(__folio_put);
>  
>  typedef void (*move_fn_t)(struct lruvec *lruvec, struct folio *folio);
>  
> +/*
> + * Unqueue a batch of folios from the deferred split queue. This must be
> + * called after the folios' refcounts have dropped to zero and before they
> + * are uncharged (see comment above __folio_unqueue_deferred_split() for why).
> + */
> +void folio_batch_unqueue_deferred_split(struct folio_batch *fbatch)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < folio_batch_count(fbatch); i++)
> +		folio_unqueue_deferred_split(fbatch->folios[i]);
> +}
> +
>  static void lru_add(struct lruvec *lruvec, struct folio *folio)
>  {
>  	int was_unevictable = folio_test_clear_unevictable(folio);
> @@ -177,14 +190,10 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
>  		 *
>  		 * We're bypassing normal cleanup. Clear flags that are not
>  		 * applicable to dead folios.
> -		 *
> -		 * Since the folio may be part of a huge page, unqueue from
> -		 * deferred split list to avoid a dangling list entry.
>  		 */
>  		if (is_lru_add && folio_ref_freeze(folio, 1)) {
>  			__folio_clear_active(folio);
>  			__folio_clear_unevictable(folio);
> -			folio_unqueue_deferred_split(folio);
>  			fbatch->folios[i] = NULL;
>  			folio_batch_add(&free_fbatch, folio);
>  			continue;
> @@ -201,6 +210,11 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
>  
>  	/* Cleanup filtered dead folios. */
>  	if (is_lru_add) {
> +		/*
> +		 * Since the folios may be part of a huge page, unqueue them
> +		 * from the deferred split list to avoid a dangling list entry.
> +		 */

In general, this lgtm, thanks. I am confused about this existing comment you are
moving.

folios being *part of* a huge page? Huh? A folio might represent a huge page,
but what does the "part of" thing mean here.

Is it simply "Dequeue any large folios" ?

-- 
Cheers,

David