Messages in this thread | | | Date | Mon, 6 Feb 2017 20:35:24 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() |
| |
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@nvidia.com> > >> > >> 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
| |