Re: [PATCH 01/15] mm/rmap: introduce anon_rmap APIs for anonymous folios

[Lorenzo Stoakes <ljs@kernel.org>]

On Wed, May 27, 2026 at 07:01:33PM +0800, tao wrote:
> Add a set of anon_rmap APIs to operate on the reverse mappings of
> anonymous folios.
>
> Introduce anon_rmap_for_each_vma() as a wrapper around
> vma_interval_tree_foreach(), so callers no longer access the
> interval tree directly.
>
> This prepares the rmap code for upcoming ANON_VMA_LAZY support and
> RCU-based lockless rmap traversal.
>
> No functional change intended.

This commit message is total garbage. You're not explaining WHY you're
using words to describe what the code does. I can read the code?

>
> Signed-off-by: tao <tao.wangtao@honor.com>

This is all horrible, horribly invasive, and adding a pile of crap on machinery
we want to get rid of.

You've added zero explanation or comments. This is just not upstreamable,
and even if you did explain yourself we don't want to extend a broken
abstraction with more broken complexity?

You're also seemingly introducing a typesafe wrapper to wrap an arbitrary value?

> ---
>  include/linux/rmap.h | 68 +++++++++++++++++++++++++++++++++++++++++
>  mm/rmap.c            | 73 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 8dc0871e5f00..c42314ea4362 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -937,6 +937,44 @@ int pfn_mkclean_range(unsigned long pfn, unsigned long nr_pages, pgoff_t pgoff,
>  void remove_migration_ptes(struct folio *src, struct folio *dst,
>  		enum ttu_flags flags);
>
> +/* Reverse mapping handle for anonymous folio rmap helpers. */
> +typedef struct anon_rmap {
> +	unsigned long rmap;
> +} anon_rmap_t;

I do not know why you're using a typedef when you just treat it as an
arbitrary value?

> +
> +#define ANON_RMAP_NULL make_anon_rmap(0)

This is just equivalent to a NULL value?...

> +
> +static inline anon_rmap_t make_anon_rmap(const void *anon_mapping)
> +{
> +	return (anon_rmap_t){ .rmap = (unsigned long)anon_mapping, };
> +}

You're intentionally defeating type safety to store arbitrary values?...

> +
> +static inline unsigned long anon_rmap_value(anon_rmap_t anon_rmap)
> +{
> +	return anon_rmap.rmap;
> +}

'Untype safe my arbitrarily type safe wrapped type'...?

> +
> +static inline anon_rmap_t anon_vma_to_anon_rmap(const struct anon_vma *anon_vma)
> +{
> +	return make_anon_rmap(anon_vma);
> +}
> +
> +static inline struct anon_vma *anon_rmap_to_anon_vma(anon_rmap_t anon_rmap)
> +{
> +	unsigned long rmap = anon_rmap_value(anon_rmap);
> +
> +	return (struct anon_vma *)rmap;
> +}

A ton of noise for seemingly no value?

> +
> +anon_rmap_t vma_get_anon_rmap(struct vm_area_struct *vma);
> +void put_anon_rmap(anon_rmap_t anon_rmap);
> +void anon_rmap_lock_write(anon_rmap_t anon_rmap);
> +int anon_rmap_trylock_write(anon_rmap_t anon_rmap);
> +void anon_rmap_unlock_write(anon_rmap_t anon_rmap);
> +void anon_rmap_lock_read(anon_rmap_t anon_rmap);
> +int anon_rmap_trylock_read(anon_rmap_t anon_rmap);
> +void anon_rmap_unlock_read(anon_rmap_t anon_rmap);

Yes let's add a bunch of extra broken abstractions on the broken abstraction.

And let's not comment anything!

> +
>  /*
>   * rmap_walk_control: To control rmap traversing for specific needs
>   *
> @@ -969,6 +1007,36 @@ void rmap_walk_locked(struct folio *folio, struct rmap_walk_control *rwc);
>  struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  					  struct rmap_walk_control *rwc);
>
> +bool folio_maybe_same_anon_vma(const struct folio *folio,
> +	const struct vm_area_struct *vma);

What the hell is this?


> +anon_rmap_t folio_get_anon_rmap(const struct folio *folio);
> +anon_rmap_t folio_lock_anon_rmap_read(const struct folio *folio,
> +	struct rmap_walk_control *rwc);
> +
> +static inline struct vm_area_struct *anon_rmap_iter_first_vma(
> +	anon_rmap_t anon_rmap, unsigned long start, unsigned long last,
> +	struct anon_vma_chain **avc)
> +{
> +	struct anon_vma *anon_vma = anon_rmap_to_anon_vma(anon_rmap);
> +
> +	*avc = anon_vma_interval_tree_iter_first(&anon_vma->rb_root, start, last);
> +	return *avc ? (*avc)->vma : NULL;
> +}

So we're allowing for folios to have NULL entries (really the commit
message should have that, rather than me scanning through uncommented
code), but in what world are we ok with an anon folio NOT BEING LINKED BACK
TO ITS VMA?

That's broken no?

> +
> +static inline struct vm_area_struct *anon_rmap_iter_next_vma(
> +	anon_rmap_t anon_rmap, unsigned long start, unsigned long last,
> +	struct anon_vma_chain **avc)
> +{
> +	if (!*avc)
> +		return NULL;
> +	*avc = anon_vma_interval_tree_iter_next(*avc, start, last);
> +	return *avc ? (*avc)->vma : NULL;
> +}
> +
> +#define anon_rmap_foreach_vma(vma, avc, anon_rmap, start, last) 	   \
> +	for (vma = anon_rmap_iter_first_vma(anon_rmap, start, last, &avc); \
> +	     vma; vma = anon_rmap_iter_next_vma(anon_rmap, start, last, &avc))
> +
>  #else	/* !CONFIG_MMU */
>
>  #define anon_vma_init()		do {} while (0)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 78b7fb5f367c..1b2dada71778 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -701,6 +701,79 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	return anon_vma;
>  }
>
> +anon_rmap_t vma_get_anon_rmap(struct vm_area_struct *vma)
> +{
> +	mmap_assert_locked(vma->vm_mm);
> +	VM_BUG_ON(!vma->anon_vma);

We don't use BUG_ON() especially VM_BUG_ON().

> +	get_anon_vma(vma->anon_vma);
> +	return anon_vma_to_anon_rmap(vma->anon_vma);
> +}
> +
> +void put_anon_rmap(anon_rmap_t anon_rmap)
> +{
> +	put_anon_vma(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +void anon_rmap_lock_write(anon_rmap_t anon_rmap)
> +{
> +	anon_vma_lock_write(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +int anon_rmap_trylock_write(anon_rmap_t anon_rmap)
> +{
> +	return anon_vma_trylock_write(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +void anon_rmap_unlock_write(anon_rmap_t anon_rmap)
> +{
> +	anon_vma_unlock_write(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +void anon_rmap_lock_read(anon_rmap_t anon_rmap)
> +{
> +	anon_vma_lock_read(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +int anon_rmap_trylock_read(anon_rmap_t anon_rmap)
> +{
> +	return anon_vma_trylock_read(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +void anon_rmap_unlock_read(anon_rmap_t anon_rmap)
> +{
> +	anon_vma_unlock_read(anon_rmap_to_anon_vma(anon_rmap));
> +}

All of these assume that you're getting an anon_vma even though you have
established above that you can put arbitrary other values?

This is terrible?

> +
> +bool folio_maybe_same_anon_vma(const struct folio *folio,
> +	const struct vm_area_struct *vma)
> +{
> +	struct anon_vma *anon_vma;
> +	struct anon_vma *tgt_anon_vma = vma->anon_vma;
> +	bool same = false;
> +
> +	rcu_read_lock();
> +	anon_vma = folio_anon_vma(folio);
> +	if (anon_vma && tgt_anon_vma)
> +		same = anon_vma->root == tgt_anon_vma->root;
> +	rcu_read_unlock();
> +	return same;

What VMA locks are being held at this point? You assert none.

Why is it maybe?

Why are you taking the RCU lock?

> +}
> +
> +anon_rmap_t folio_get_anon_rmap(const struct folio *folio)
> +{
> +	struct anon_vma *anon_vma = folio_get_anon_vma(folio);
> +
> +	return anon_vma ? anon_vma_to_anon_rmap(anon_vma) : ANON_RMAP_NULL;
> +}
> +
> +anon_rmap_t folio_lock_anon_rmap_read(const struct folio *folio,
> +				      struct rmap_walk_control *rwc)
> +{
> +	struct anon_vma *anon_vma = folio_lock_anon_vma_read(folio, rwc);
> +
> +	return anon_vma ? anon_vma_to_anon_rmap(anon_vma) : ANON_RMAP_NULL;
> +}
> +
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>  /*
>   * Flush TLB entries for recently unmapped pages from remote CPUs. It is
> --
> 2.17.1
>