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

[xu.xin16]

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

Yes, we cannot rely solely on anon_vma to locate all PTEs mapping this page; we
must also have the original page's pgoff. In fact, I believe only the current
vma->vm_pgoff is just necessary. I've examined the implementation of
anon_vma_interval_tree_foreach — it essentially iterates to find a suitable
VMA such that the provided pgoff falls within the VMA's range
	[vm_pgoff, vm_pgoff + vma_pages(v) - 1].

The root cause of the issue Hugh points is that the pgoff calculated from
rmap_item->address (which derives from vma->vm_start) is not the pgoff of
the page prior to merging. Consequently, the anon_vma_interval_tree_foreach traversal
cannot match the correct VMA satisfying vma_start_pgoff <= pgoff <= vma_end_pgoff.
This origins from an existing fact: if a user invokes mremap(), the new vma->vm_start
may changes, while the mapped page's index remains unchanged, but vma->vm_pgoff is updated
synchronously to ensure that the vma_address() calculation remains valid, like
rmap_walk_anon() in mm/rmap.c.

Based on the above, I think a simpler approach which does not increase the size
of the ksm_rmap_item struct below.

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

Can we just replace the stored anon_vma of "ksm_rmap_item" with the orig_vma
when KSM merging? Then, from rmap_item->orig_vma, we can directly obtain both
the anon_vma and the vm_pgoff, thereby enabling the location of all PTEs mapping
this page without any ambiguity.

Cheers, Xu


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