Re: [PATCH 1/4] rmap: introduce rmap_walk_locked()

[Kirill A. Shutemov]

On Wed, Feb 03, 2016 at 02:56:07PM -0800, Andrew Morton wrote:
> On Thu, 4 Feb 2016 01:45:07 +0300 "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
> 
> > On Wed, Feb 03, 2016 at 02:40:19PM -0800, Andrew Morton wrote:
> > > On Wed,  3 Feb 2016 18:14:16 +0300 "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
> > > 
> > > > rmap_walk_locked() is the same as rmap_walk(), but caller takes care
> > > > about relevant rmap lock. It only supports anonymous pages for now.
> > > > 
> > > > It's preparation to switch THP splitting from custom rmap walk in
> > > > freeze_page()/unfreeze_page() to generic one.
> > > > 
> > > > ...
> > > >
> > > > +/* Like rmap_walk, but caller holds relevant rmap lock */
> > > > +int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
> > > > +{
> > > > +	/* only for anon pages for now */
> > > > +	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
> > > > +	return rmap_walk_anon(page, rwc, true);
> > > > +}
> > > 
> > > Should be rmap_walk_anon_locked()?
> > 
> > I leave interface open for further extension for file mappings, once it
> > will be needed. Interface is mirroring plain rmap_walk()
> 
> hm, yes, I see.
> 
> > If you prefer to rename the function, I can do it too.
> 
> Well, what does "unlocked" mean in the context of rmap_walk_ksm() and
> rmap_walk_file()?

For rmap_walk_file(), caller should take i_mmap_lock for page->mapping at
least for read.

Not sure about KSM..

> That the caller holds totally different locks.  I expect that sitting
> down and writing out the interface definition for such an
> rmap_walk_locked() would reveal that we shouldn't have created it.
> 
> I mean, if the caller is to call such an rmap_walk_locked(), he first
> needs to work out if it's a ksm page or an anon page or a file page,
> then take the appropriate lock and then call rmap_walk_locked(). 
> That's silly - at this point he should directly call
> rmap_walk_ksm_locked()?

It makes sense if you have multiple pages to process and it's known that
they share reverse mapping.

Or if you want to keep the reverse mapping locked to keep continuity with
other operations.

In THP case, we have 512 subpages to unmap and we want to keep anon_vma
locked until the THP is split.

-- 
 Kirill A. Shutemov