Re: [PATCH 02/25] mm/fbatch: allow folios_put_refs() to skip xa_is_value() entries

[David Hildenbrand (Arm)]

On 8/24/26 15:55, Hugh Dickins wrote:
> Let folios_put_refs() (hence folio_batch_release()) skip xa_is_value()
> entries, and therefore remove unneeded folio_batch_remove_exceptionals().
> 
> It made some sense when introduced in 3.1 for shmem swap entries only,
> but workingset shadows popularized exceptional entries in 3.15, and it's
> silly for so many sites to be squashing exceptionals out of the fbatch,
> merely to suit an inadequacy in folios_put_refs().
> 
> But remove exceptionals on leaving truncate_folio_batch_exceptionals(),
> one of whose callers then passes the fbatch on to others less tolerant.
> 
> No longer essential to this series, since 7.2 commit 9669b87065a6
> ("mm/lruvec: preemptively free dead folios during lru_add drain")
> allowed folios_put_refs() to skip NULLs; but still an improvement.
> 
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> ---
>  include/linux/folio_batch.h |  5 +----
>  mm/folio.c                  | 25 ++++---------------------
>  mm/shmem.c                  |  2 --
>  mm/truncate.c               | 16 ++++++++++------
>  4 files changed, 15 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/folio_batch.h b/include/linux/folio_batch.h
> index b45946adc50b..e1cc8ae023f1 100644
> --- a/include/linux/folio_batch.h
> +++ b/include/linux/folio_batch.h
> @@ -22,8 +22,7 @@ struct folio;
>   * The folio_batch is used to amortise the cost of retrieving and
>   * operating on a set of folios.  The order of folios in the batch may be
>   * significant (eg delete_from_page_cache_batch()).  Some users of the
> - * folio_batch store "exceptional" entries in it which can be removed
> - * by calling folio_batch_remove_exceptionals().
> + * folio_batch store "exceptional" (xa_is_value) entries in it too.
>   */
>  struct folio_batch {
>  	unsigned char nr;
> @@ -100,6 +99,4 @@ static inline void folio_batch_release(struct folio_batch *fbatch)
>  	if (folio_batch_count(fbatch))
>  		__folio_batch_release(fbatch);
>  }
> -
> -void folio_batch_remove_exceptionals(struct folio_batch *fbatch);
>  #endif /* _LINUX_FOLIO_BATCH_H */
> diff --git a/mm/folio.c b/mm/folio.c
> index 62b96c9ce19e..b2bce6b77498 100644
> --- a/mm/folio.c
> +++ b/mm/folio.c
> @@ -983,6 +983,10 @@ void folios_put_refs(struct folio_batch *folios, unsigned int *refs)
>  		if (!folio)
>  			continue;
>  
> +		/* Skip any "exceptional" (workingset or shmem swap) entry. */
> +		if (xa_is_value(folio))
> +			continue;
> +
>  		if (is_huge_zero_folio(folio))
>  			continue;
>  
> @@ -1088,27 +1092,6 @@ void __folio_batch_release(struct folio_batch *fbatch)
>  }
>  EXPORT_SYMBOL(__folio_batch_release);
>  
> -/**
> - * folio_batch_remove_exceptionals() - Prune non-folios from a batch.
> - * @fbatch: The batch to prune
> - *
> - * find_get_entries() fills a batch with both folios and shadow/swap/DAX
> - * entries.  This function prunes all the non-folio entries from @fbatch
> - * without leaving holes, so that it can be passed on to folio-only batch
> - * operations.
> - */
> -void folio_batch_remove_exceptionals(struct folio_batch *fbatch)
> -{
> -	unsigned int i, j;
> -
> -	for (i = 0, j = 0; i < folio_batch_count(fbatch); i++) {
> -		struct folio *folio = fbatch->folios[i];
> -		if (!xa_is_value(folio))
> -			fbatch->folios[j++] = folio;
> -	}
> -	fbatch->nr = j;
> -}
> -
>  #ifdef CONFIG_MEMCG
>  static void lruvec_reparent_lru(struct lruvec *child_lruvec,
>  				struct lruvec *parent_lruvec,
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 89a1495e55f7..3911721e1e55 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1156,7 +1156,6 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, uoff_t lend,
>  				truncate_inode_folio(mapping, folio);
>  			folio_unlock(folio);
>  		}
> -		folio_batch_remove_exceptionals(&fbatch);
>  		folio_batch_release(&fbatch);
>  		cond_resched();
>  	}
> @@ -1276,7 +1275,6 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, uoff_t lend,
>  			}
>  			folio_unlock(folio);
>  		}
> -		folio_batch_remove_exceptionals(&fbatch);
>  		folio_batch_release(&fbatch);
>  	}
>  
> diff --git a/mm/truncate.c b/mm/truncate.c
> index b58ba940be47..4151f7a167e3 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -53,7 +53,7 @@ static void clear_shadow_entries(struct address_space *mapping,
>  /*
>   * Unconditionally remove exceptional entries. Usually called from truncate
>   * path. Note that the folio_batch may be altered by this function by removing
> - * exceptional entries similar to what folio_batch_remove_exceptionals() does.
> + * exceptional entries.
>   * Please note that indices[] has entries in ascending order as guaranteed by
>   * either find_get_entries() or find_lock_entries().
>   */
> @@ -95,7 +95,7 @@ static void truncate_folio_batch_exceptionals(struct address_space *mapping,
>  				dax_delete_mapping_entry(mapping, indices[i]);
>  			}
>  		}
> -		goto out;
> +		goto squash;
>  	}
>  
>  	xas_set(&xas, indices[j]);
> @@ -113,8 +113,14 @@ static void truncate_folio_batch_exceptionals(struct address_space *mapping,
>  	if (mapping_shrinkable(mapping))
>  		inode_lru_list_add(mapping->host);
>  	spin_unlock(&mapping->host->i_lock);
> -out:
> -	folio_batch_remove_exceptionals(fbatch);
> +
> +squash:
> +	for (i = j + 1; i < nr; i++) {
> +		folio = fbatch->folios[i];
> +		if (!xa_is_value(folio))
> +			fbatch->folios[j++] = folio;
> +	}
> +	fbatch->nr = j;
>  }

One caller of truncate_folio_batch_exceptionals() just calls
folio_batch_release(). I was wondering whether that one could just stop calling
truncate_folio_batch_exceptionals().

But truncate_folio_batch_exceptionals() does too many other things I don't quite
understand.

I guess one could move the loop into the only remaining caller that does not
call folio_batch_release() afterwards.

But that changes the whole purpose of the function and the name would no longer
match.

Which brings me back to my point that truncate_folio_batch_exceptionals() does a
lot of things besides folio_batch_remove_exceptionals() that I don't understand :)

So I'm sure there is room for future cleanups for somebody that actually
understands what truncate_folio_batch_exceptionals() does besides ... removing
exceptionals.

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David