Re: [PATCH 0/4] Some more lock_page work..

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Wed, Oct 14, 2020 at 07:15:09PM +0100, Matthew Wilcox wrote:
> On Wed, Oct 14, 2020 at 09:53:35AM -0700, Linus Torvalds wrote:
> > In particular, what I _think_ we could do is:
> > 
> >  - lock the page tables
> > 
> >  - check that the page isn't locked
> > 
> >  - increment the page mapcount (atomic and ordered)
> > 
> >  - check that the page still isn't locked
> > 
> >  - insert pte
> > 
> > without taking the page lock. And the reason that's safe is that *if*
> [...]
> > And they aren't necessarily a _lot_ more involved. In fact, I think we
> > may already hold the page table lock due to doing that
> > "pte_alloc_one_map()" thing over all of filemap_map_pages(). So I
> > think the only _real_ problem is that I think we increment the
> > page_mapcount() too late in alloc_set_pte().
> 
> I'm not entirely sure why we require the page lock to be held in
> page_add_file_rmap():
> 
>         } else {
>                 if (PageTransCompound(page) && page_mapping(page)) {
>                         VM_WARN_ON_ONCE(!PageLocked(page));
> 
>                         SetPageDoubleMap(compound_head(page));
>                         if (PageMlocked(page))
>                                 clear_page_mlock(compound_head(page));
>                 }
> 
> We have a reference to the page, so compound_head() isn't going
> to change.  SetPageDoubleMap() is atomic.  PageMlocked() is atomic.
> clear_page_mlock() does TestClearPageMlocked() as its first thing,
> so that's atomic too.  What am I missing?  (Kirill added it, so I
> assume he remembers ;-)

In general (with current scheme), we need page lock here to serialize
against try_to_unmap() and other rmap walkers.

For file THP, we also serialize against follow_trans_huge_pmd(FOLL_MLOCK).

-- 
 Kirill A. Shutemov