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

From: KAMEZAWA Hiroyuki
Date: Mon Jan 09 2012 - 22:46:05 EST


On Fri, 6 Jan 2012 22:07:29 +0800
Hillf Danton <dhillf@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
> ---
>
> --- 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/