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

[wangtao <tao.wangtao@honor.com>]

> 
> 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?
> 

I thought these were just simple wrappers, so they seemed
straightforward, so I added them in the new patch.

> > ---
> >  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?
> 
anon_rmap_t provides functionality externally, so using a struct
is relatively more robust and helps prevent misuse.

> > +
> > +#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?
> 
I’m not sure I understand your question. Are you asking whether we should check that *avc is non‑NULL?

Here, anon_rmap_foreach_vma() is used to replace anon_vma_interval_tree_foreach.
After obtaining avc, we then check that it is non‑NULL.

#define anon_vma_interval_tree_foreach(avc, root, start, last)		 \
	for (avc = anon_vma_interval_tree_iter_first(root, start, last); \
	     avc; avc = anon_vma_interval_tree_iter_next(avc, start, last))


> > +
> > +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?
> 

Using only anon_vma->root is just a simple preliminary check; it is
necessary to obtain the PTE to determine whether the page is actually
used by this anon_vma

The anon_vma obtained from folio_anon_vma(folio) must be accessed
under RCU; otherwise it may already have been freed.