Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

[Kirill A. Shutemov]

On Mon, Feb 06, 2017 at 10:32:10AM -0600, Zi Yan wrote:
> On 6 Feb 2017, at 10:07, Kirill A. Shutemov wrote:
> 
> > On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> >> From: Zi Yan <ziy@xxxxxxxxxx>
> >>
> >> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> >> This can cause pmd_protnone entry not being freed.
> >>
> >> Because there are two steps in changing a pmd entry to a pmd_protnone
> >> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> >> the pmd_none entry is changed into a pmd_protnone entry.
> >> The racy check, even with barrier, might only see the pmd_none entry
> >> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
> >
> > That's definately a good catch.
> >
> > But I don't agree with the solution. Taking pmd lock on each
> > zap_pmd_range() is a significant hit by scalability of the code path.
> > Yes, split ptl lock helps, but it would be nice to avoid the lock in first
> > place.
> >
> > Can we fix change_huge_pmd() instead? Is there a reason why we cannot
> > setup the pmd_protnone() atomically?
> 
> If you want to setup the pmd_protnone() atomically, we need a new way of
> changing pmds, like pmdp_huge_cmp_exchange_and_clear(). Otherwise, due to
> the nature of racy check of pmd in zap_pmd_range(), it is impossible to
> eliminate the chance of catching this bug if pmd_protnone() is setup
> in two steps: first, clear it, second, set it.
> 
> However, if we use pmdp_huge_cmp_exchange_and_clear() to change pmds from now on,
> instead of current two-step approach, it will eliminate the possibility of
> using batched TLB shootdown optimization (introduced by Mel Gorman for base page swapping)
> when THP is swappable in the future. Maybe other optimizations?

I'll think about this more.

> Why do you think holding pmd lock is bad?

Each additional atomic operation in fast-path hurts scalability.
Cost of atomic operations rises fast as machine gets bigger.

> In zap_pte_range(), pte lock is also held when each PTE is zapped.

It's necessary evil for pte. Not so much for pmd so far.

> BTW, I am following Naoya's suggestion and going to take pmd lock inside
> the loop. So pmd lock is held when each pmd is being checked and it will be released
> when the pmd entry is zapped, split, or pointed to a page table.
> Does it still hurt much on performance?

Naoya's suggestion is not correct: pmd_lock() can be different not for
each pmd entry, but for each pmd table. So taking it outside of the loop
is correct.


-- 
 Kirill A. Shutemov