Re: [PATCH v3 2/2] ksm: Optimize rmap_walk_ksm by passing a suitable address range

[Lorenzo Stoakes]

On Wed, Apr 08, 2026 at 02:57:10PM +0200, David Hildenbrand (Arm) wrote:
> On 4/7/26 11:36, Lorenzo Stoakes (Oracle) wrote:
> > On Tue, Apr 07, 2026 at 02:21:41PM +0800, xu.xin16@xxxxxxxxxx wrote:
> >>>
> >>> I'd completely forgotten that patch by now!  But it's dealing with a
> >>> different issue; and note how it's intentionally leaving MADV_MERGEABLE
> >>> on the vma itself, just using MADV_UNMERGEABLE (with &dummy) as an
> >>> interface to CoW the KSM pages at that time, letting them be remerged after.
> >
> > Hmm yeah, we mark them unmergeable but don't update the VMA flags (since using
> > &dummy), so they can just be merged later right?
> >
> > And then the:
> >
> > void rmap_walk_ksm(struct folio *folio, struct rmap_walk_control *rwc)
> > {
> > 	...
> > 		const pgoff_t pgoff = rmap_item->address >> PAGE_SHIFT;
> > 		...
> > 		anon_vma_interval_tree_foreach(vmac, &anon_vma->rb_root,
> > 					       pgoff, pgoff) {
> > 			...
> > 		}
> > 	...
> > }
> >
> > Would _assume_ that folio->pgoff == addr >> PAGE_SHIFT, which will no longer be
> > the case here?
>
> I'm wondering whether we could figure the pgoff out, somehow, so we
> wouldn't have to store it elsewhere.
>
> What we need is essentially what __folio_set_anon() would have done for
> the original folio we replaced.
>
> 	folio->index = linear_page_index(vma, address);
>
> Could we obtain that from the anon_vma assigned to our rmap_item?
>
> pgoff_t pgoff;
>
> pgoff = (rmap_item->address - anon_vma->vma->vm_start) >> PAGE_SHIFT;
> pgoff += anon_vma->vma->vm_pgoff;

anon_vma doesn't have a vma field :) it has anon_vma->rb_root which maps to all
'related' VMAs.

And we're already looking at what might be covered by the anon_vma by
invoking anon_vma_interval_tree_foreach() on anon_vma->rb_root in [0,
ULONG_MAX).

>
> It would be the same adjustment everywhere we look in child processes,
> because the moment they would mremap() would be where we would have
> unshared.
>
> Just a thought after reading avc_start_pgoff ...

One interesting thing here is in the anon_vma_interval_tree_foreach() loop
we check:

if (addr < vma->vm_start || addr >= vma->vm_end)
	continue;

Which is the same as saying 'hey we are ignoring remaps'.

But... if _we_ got remapped previously (the unsharing is only temporary),
then we'd _still_ have an anon_vma with an old index != addr >> PAGE_SHIFT,
and would still not be able to figure out the correct pgoff after sharing.

I wonder if we could just store the pgoff in the rmap_item though?

Because we unshare on remap, so we'd expect a new share after remapping, at
which point we could account for the remapping by just setting
rmap_item->pgoff = vma->vm_pgoff I think?

Then we're back in business.

Another way around this issue is to do the rmap_walk_ksm() loop for (addr
>> PAGE_SHIFT) _first_, but that'd only be useful for walkers that can exit
early once they find the mapping they care about, and I worry about 'some
how' missing remapped cases, so probably not actually all that useful.

>
> --
> Cheers,
>
> David

Cheers, Lorenzo