Re: [PATCH] mm: vmscan: recompute page status when putting back

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Fri, 6 Jan 2012 22:07:29 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> If unlikely the given page is isolated from lru list again, its status is
> recomputed before putting back to lru list, since the comment says page's
> status can change while we move it among lru.
> 
> 
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/mm/vmscan.c	Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c	Fri Jan  6 21:31:56 2012
> @@ -633,12 +633,14 @@ int remove_mapping(struct address_space
>  void putback_lru_page(struct page *page)
>  {
>  	int lru;
> -	int active = !!TestClearPageActive(page);
> -	int was_unevictable = PageUnevictable(page);
> +	int active;
> +	int was_unevictable;
> 
>  	VM_BUG_ON(PageLRU(page));
> 
>  redo:
> +	active = !!TestClearPageActive(page);
> +	was_unevictable = PageUnevictable(page);
>  	ClearPageUnevictable(page);
> 
>  	if (page_evictable(page, NULL)) {

Hm. Do you handle this case ?
==
        /*
         * page's status can change while we move it among lru. If an evictable
         * page is on unevictable list, it never be freed. To avoid that,
         * check after we added it to the list, again.
         */
        if (lru == LRU_UNEVICTABLE && page_evictable(page, NULL)) {
                if (!isolate_lru_page(page)) {
         		put_page(page);
                        goto redo;
                }			
==

Ok, let's start from "was_unevictable"

"was_unevicatable" is used for this
==
  if (was_unevictable && lru != LRU_UNEVICTABLE)
                count_vm_event(UNEVICTABLE_PGRESCUED);
==
This is for checking that the page turned out to be evictable while we put it
into LRU. Assume the 'redo' case, the page's state chages from UNEVICTABLE to
ACTIVE_ANON (for example)

  1. at start of function: Page was Unevictable, was_unevictable=true
  2. lru = LRU_UNEVICTABLE
  3, add the page to LRU.
  4. check page_evictable(),..... it returns 'true'.
  5. isoalte the page again and goto redo.
  6. lru = LRU_ACTIVE_ANON
  7. add the page to LRU.
  8. was_unevictable==true, then, count_vm_event(UNEVICTABLE_PGRESCUED);

Your patch overwrites was_unevictable between 5. and 6., then, 
corrupts this event counting.

about "active" flag.

PageActive() flag will be set in lru_cache_add_lru() and
there will be no inconsistency between page->flags and LRU.
And, in what case the changes in 'active' will be problematic ?

-Kame













--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>