* Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx> [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@xxxxxxxxxxxxx> 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? > > > > - only now does CPU2 get to the "pte_get_and_clear()" to remove one page > > > > - CPU2 does a TLB flush and frees the page > > > > End result: > > > > - both CPU1 _and_ CPU2 have flushed the TLB. > > > > - but both flushed the *OLD* address > > > > - the page is freed > > > > - CPU3 still has the stale TLB entry pointing to the page that is now > > free and might be reused for something else > > > > Am I missing something? > > > > That is a problem. With that it looks like CONFIG_HAVE_MOVE_PMD/PUD is > broken? I don't see an easy way to fix this? > > -aneesh > >
