On Thu, 16 Sep 2010, Andrea Arcangeli wrote: > On Wed, Sep 15, 2010 at 05:10:36PM -0700, Hugh Dickins wrote: > > I agree that if my scenario happened on its own, the pte_same check > > would catch it. But if my scenario happens along with your scenario > > (and I'm thinking that the combination is not that much less likely > > than either alone), then the PageSwapCache test will succeed and the > > pte_same test will succeed, but we're still putting the wrong page into > > the pte, since this page is now represented by a different swap entry > > (and the page that should be there by our original swap entry). > > If I understood well you're saying that it is possible that this > BUG_ON triggers: > > page_table = pte_offset_map_lock(mm, pmd, address, &ptl); > BUG_ON(page_private(page) != entry.val && pte_same(*page_table, orig_pte)); > if (unlikely(!pte_same(*page_table, orig_pte))) Yes, I believe so. > > I still don't get it (that doesn't make me right though). > > I'll try to rephrase my argument: if the page was swapped in from > swapcache by swapoff and then swapon runs again and the page is added > to swapcache to a different swap entry, in between the > lookup_swap_cache and the lock_page, the pte_same(*page_table, > orig_pte) in pte_same should always fail in the first place (so > without requiring the page_private(page) != entry.val check). Usually yes, but more may have happened in between. > > If the page is found mapped during pte_same the pte_same check will > fail (pte_present first of all). If the page got unmapped and > page_private(page) != entry.val, the "entry" == "orig_pte" will be > different to what we read in *page_table at the above BUG_ON line (the > page has to be unmapped before pte_same check can succeed, but if gets > unmapped the new swap entry will be written in the page_table and it > won't risk to succeed the pte_same check). Usually yes, but not necessarily. > > If the page wasn't mapped when it was removed from swapcache, it can't > be added to swapcache at all because it was pinned: because only free > pages (during swapin) or mapped pages (during swapout) can be added to > swapcache. Yes, I think that happens to be the case, but does not rule out my scenario. Perhaps there's a page_count test that I've overlooked that makes my scenario impossible, but is_page_cache_freeable() appears to prevent writeout without affecting swap allocation. > > If I'm missing something a trace of the exact scenario would help to > clarify your point. Indeed yes: I was being lazy, hoping to get you to do my thinking for me (in my defence, and in your praise, I have to say that that is usually much the quickest strategy :-) Thank you for the time you've spent on it, when I should have tried harder. Here's what I think can happen: you may shame me by shooting it down immediately, but go ahead! I've cast it in terms of reuse_swap_page(), but I expect it could be reformulated to rely on try_to_free_swap() instead, or swapoff+swapon. A, in do_swap_page(): does page1 = lookup_swap_cache(swap1) and comes through the lock_page(page1). B, a racing thread of same process, also faults into do_swap_page(): does page1 = lookup_swap_cache(swap1) and now waits in lock_page(page1), but for whatever reason is unlucky not to get the lock any time soon. A carries on through do_swap_page(), a write fault, but cannot reuse the swap page1 (another reference to swap1). Unlocks the page1 (but B doesn't get it yet), does COW in do_wp_page(), page2 now in that pte. C, perhaps the parent of A+B, comes in and write faults the same swap page1 into its mm, reuse_swap_page() succeeds this time, swap1 is freed. kswapd comes in after some time (B still unlucky) and swaps out some pages from A+B and C: it allocates the original swap1 to page2 in A+B, and some other swap2 to the original page1 now in C. But does not immediately free page1 (actually it couldn't: B holds a reference), leaving it in swap cache for now. B at last gets the lock on page1, hooray! Is PageSwapCache(page1)? Yes. Is pte_same(*page_table, orig_pte)? Yes, because page2 has now been given the swap1 which page1 used to have. So B proceeds to insert page1 into A+B's page_table, though its content now belongs to C, quite different from what A wrote there. B ought to have checked that page1's swap was still swap1. Hugh -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxxx For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>