lkml.org 
[lkml]   [2008]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2008-05-13 14:43    [W:1.344 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site