Re: [PATCH -mm 05/15] free swap space on swap-in/activation

[Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>]

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.