RE: [PATCH 02/15] mm: convert anon_vma rmap APIs to anon_rmap

[wangtao]

> Subject: Re: [PATCH 02/15] mm: convert anon_vma rmap APIs to anon_rmap
> 
> On Wed, May 27, 2026 at 07:01:34PM +0800, tao wrote:
> > Convert the rmap anon_vma interfaces to anon_rmap APIs to clarify the
> > semantics of anonymous rmap operations and prepare for upcoming
> > ANON_VMA_LAZY support and RCU-based lockless rmap traversal.
> >
> > Replace folio_anon_vma(), folio_get_anon_vma(),
> > folio_lock_anon_vma_read(), anon_vma_trylock_read(),
> > anon_vma_lock_read(), anon_vma_unlock_read(),
> > anon_vma_trylock_write(), anon_vma_lock_write(),
> anon_vma_unlock_write(), and vma_interval_tree_foreach() with the
> anon_rmap APIs.
> 
> This is another worthless commit message. You're again just writing what you
> did not why or what for. This gives no help whatsoever.
> 
I will update the commit message to add more explanation.

> >
> > No functional change intended.
> 
> Err, there is a functional change, since you're literally changing how things
> iterate?
> 
> >
> > Signed-off-by: tao <tao.wangtao@xxxxxxxxx>
> 
> All of this is terrible, you're replacing a broken abstraction with something
> that assumes something completely broken with zero explanation.
> 
> No to this.
> 
Before supporting ANON_VMA_LAZY, these were indeed just simple wrappers.

> >  /*
> > @@ -3035,9 +3040,10 @@ static struct anon_vma
> > *rmap_walk_anon_lock(const struct folio *folio,  static void
> rmap_walk_anon(struct folio *folio,
> >  		struct rmap_walk_control *rwc, bool locked)  {
> > -	struct anon_vma *anon_vma;
> > +	anon_rmap_t anon_rmap;
> >  	pgoff_t pgoff_start, pgoff_end;
> >  	struct anon_vma_chain *avc;
> > +	struct vm_area_struct *vma;
> 
> I have no idea why you put the VMA at this scope...

The vma is used in anon_rmap_foreach_vma().

> 
> >
> >  	/*
> >  	 * The folio lock ensures that folio->mapping can't be changed under
> > us @@ -3046,20 +3052,19 @@ static void rmap_walk_anon(struct folio
> *folio,
> >  	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> >
> >  	if (locked) {
> > -		anon_vma = folio_anon_vma(folio);
> > +		anon_rmap = folio_anon_rmap(folio);
> >  		/* anon_vma disappear under us? */
> > -		VM_BUG_ON_FOLIO(!anon_vma, folio);
> > +		VM_BUG_ON_FOLIO(!anon_rmap_value(anon_rmap), folio);
> >  	} else {
> > -		anon_vma = rmap_walk_anon_lock(folio, rwc);
> > +		anon_rmap = rmap_walk_anon_lock(folio, rwc);
> >  	}
> > -	if (!anon_vma)
> > +	if (!anon_rmap_value(anon_rmap))
> >  		return;
> >
> >  	pgoff_start = folio_pgoff(folio);
> >  	pgoff_end = pgoff_start + folio_nr_pages(folio) - 1;
> > -	anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
> > +	anon_rmap_foreach_vma(vma, avc, anon_rmap,
> >  			pgoff_start, pgoff_end) {
> > -		struct vm_area_struct *vma = avc->vma;
> 
> Don't throw random changes like this in with a general replacement patch.

These were replaced carefully, but there may still be omissions or mistakes. Please point them out specifically.
> 
> >  		unsigned long address = vma_address(vma, pgoff_start,
> >  				folio_nr_pages(folio));
> >
> > @@ -3076,7 +3081,7 @@ static void rmap_walk_anon(struct folio *folio,
> >  	}
> >
> >  	if (!locked)
> > -		anon_vma_unlock_read(anon_vma);
> > +		anon_rmap_unlock_read(anon_rmap);
> >  }
> >
> >  /**
> > --
> > 2.17.1
> >