Messages in this thread | | | Date | Tue, 13 May 2008 21:39:30 +0900 | From | Daisuke Nishimura <> | Subject | Re: [PATCH -mm 05/15] free swap space on swap-in/activation |
| |
On 2008/05/12 22:51 +0900, Lee Schermerhorn wrote: > On Mon, 2008-05-12 at 20:21 +0900, Daisuke Nishimura wrote: >> Hi. >> >> I have a question about the intention of this patch. >> >> I think all the remove_exclusive_swap_page() you have added >> are called while the _count of the page is incremented by >> __isolate_lru_page(). >> >> So, IMHO, swap caches that will be freed by this patch should have >> page counts from __isolate_lru_page() and the swap cache itself. >> >> They are different from, for example, those are freed by do_swap_page() >> -> remove_exclusive_swap_page(), that is, swap caches >> that have been just mapped and have page counts from >> the process(only user of the page) and the swap cache itself. >> >> So, the intention of this patch is >> not to free a swap space of swap cache that has already swapped in >> and only used one process as do_swap_page() does, >> but to free a swap space that is only used as a swap cache >> (not used by any processes), right? > > I agree. I had noticed this when I wanted to use > remove_exclusive_swap_page() when a page in the swap cache became > mlocked, but I hadn't got to ask about it yet. As you note, we hold an > extra reference in that case. Further more, an anonymous page, in the > swap cache, can be shared, read-only by multiple tasks--ancestors and > descendants. So, I think the check could be changed to something like: > > (page_count(page) - page_mapcount(page) - extra_ref) == 1 to remove, or > '!= 1' to reject removal. > > "extra_ref" would be a new argument to remove_exclusive_swap_page() that > would be passed as 0 in do_swap_page() and, I think, free_swap_cache() > and swap_writepage()--current callers. In Rik's patch and when mlocking > a page, we'd pass extra_ref as 1 or 2 because we hold the extra ref from > isolate_lru_page and maybe one from get_user_pages() in the mlock case. > > Thoughts? >
I agree that the check in remove_excusive_swap_page() should be changed, and your new check seems rational for me.
But I have one comment.
I think swap_writepage(), which is one of the current caller, should not call remove_exclusive_swap_page() with extra_ref=0, because swap_writepage() can be called via shrink_page_list() -> pageout(), that is, while the page count is incremented by page isolation. If there is a swap cache page that would be freed in current shrink_page_list() -> pageout() -> swap_writepage() -> remove_exclusive_swap_page() sequence, the page count of the page is 2(page isolation and swap cache). But if you modify remove_exclusive_swap_page() and call it with extra_ref=0, the condition isn't met. I don't know wether swap_writepage() would be called via other sequence or not, but it should not call remove_exclusive_swap_page() simply with extra_ref=0.
Thanks, Daisuke Nishimura.
| |