Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 5/21/21 8:54 PM, Liam Howlett wrote:
> * Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> [210521 08:51]:
>> On 5/21/21 11:43 AM, Linus Torvalds wrote:
>>> On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
>>> <aneesh.kumar@linux.ibm.com> wrote:
>>>>
>>>> On 5/21/21 8:10 AM, Linus Torvalds wrote:
>>>>>
>>>>> So mremap does need to flush the TLB before releasing the page table
>>>>> lock, because that's the lifetime boundary for the page that got
>>>>> moved.
>>>>
>>>> How will we avoid that happening with
>>>> c49dd340180260c6239e453263a9a244da9a7c85 /
>>>> 2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
>>>> performance by moving level3/level2 page table entries. When doing so we
>>>> are not holding level 4 ptl lock (pte_lock()). But rather we are holding
>>>> pmd_lock or pud_lock(). So if we move pages around without holding the
>>>> pte lock, won't the above issue happen even if we do a tlb flush with
>>>> holding pmd lock/pud lock?
>>>
>>> Hmm. Interesting.
>>>
>>> Your patch (to flush the TLB after clearing the old location, and
>>> before inserting it into the new one) looks like an "obvious" fix.
>>>
>>> But I'm putting that "obvious" in quotes, because I'm now wondering if
>>> it actually fixes anything.
>>>
>>> Lookie here:
>>>
>>>    - CPU1 does a mremap of a pmd or pud.
>>>
>>>       It clears the old pmd/pud, flushes the old TLB range, and then
>>> inserts the pmd/pud at the new location.
>>>
>>>    - CPU2 does a page shrinker, which calls try_to_unmap, which calls
>>> try_to_unmap_one.
>>>
>>> These are entirely asynchronous, because they have no shared lock. The
>>> mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
>>> which does the pte lock.
>>>
>>> Now, imagine that the following ordering happens with the two
>>> operations above, and a CPU3 that does accesses:
>>>
>>>    - CPU2 follows (and sees) the old page tables in the old location and
>>> the took the pte lock
>>>
>>>    - the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
>>> *and* inserts in the new place.
>>>
>>>    - a user thread on CPU3 accesses the new location and fills the TLB
>>> of the *new* address
> 
> mremap holds the mmap_sem in write mode as well, doesn't it?  How is the user thread
> getting the new location?
> 

Immediately after CPU1 insert new addr translation as part of mremap, 
CPU3 can access that translation by dereferencing the new address.

-aneesh