On Fri, Mar 26, 2010 at 09:32:28PM +0000, Hugh Dickins wrote: > It's a nice little catch, but you certainly shouldn't have buried it > amidst 40 other patches unrelated to it! Yep ;). > I was going to ack that patch and urge you to forward it to Andrew > separately, but did you test whether it works? Isn't it actually a > no-op? Because KSM is holding page lock across replace_page() (maybe > that's something I added after you were last there - at the time I did > it just from instinct, and to make a block look prettier; but later I > believe it turned out to be necessary), and free_swap_cache() only > works if trylock_page() succeeds. > > So if we want the fix, I think it would have to be reworked, perhaps > slightly messier. An equivalent patch (a lot messier like you said) was tested and verified for the pre-swap release). Earlier kernels would do nothing in free_page_and_swap_cache unless it was the last reference (they checked page_count(page) != 2, instead of page_mapped()). This is why it was messier in the earlier version for earlier kernel. But I thought doing this there would work and it would be cleaner too by taking advantage of the mapcount check in free_page_and_swap_cache internals. In the earlier version I'm not doing it inside replace_page so the page lock wouldn't be a problem anyway, but the older version that I'm more familiar with, only takes the PG_lock during page_wrprotect! Why do you keep the PG_lock during replace_page too? do_wp_page can't run as we re-verify the pte didn't change. Maybe is just to be safer? > Something needed for -stable? No, I think not, it's a situation that Not needed, it's just for statistics and to speedup ksm so that it won't require to invoke the page reclaim to collect orhpaned swapcache that has to go away immediately. > gradually increases memory pressure, and then memory pressure frees it > (vmscan.c's __delete_from_swap_cache); it never prevents swapoff. > > But it would be nice to fix it all the same: thanks for spotting. It wasn't me spotting it, I only fixed it for the earlier version and ported to mainline after verifying the fix works. Thanks to you for noticing that replace_page now runs under page lock ;). Andrea -- 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>