lkml.org 
[lkml]   [2008]   [May]   [13]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
    /
    DateTue, 13 May 2008 21:39:30 +0900
    FromDaisuke Nishimura <>
    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    [from the cache]
    ©2003-2008