* [PATCH] mm: vmscan: recompute page status when putting back
@ 2012-01-06 14:07 Hillf Danton
2012-01-09 16:49 ` KOSAKI Motohiro
2012-01-10 3:44 ` KAMEZAWA Hiroyuki
0 siblings, 2 replies; 4+ messages in thread
From: Hillf Danton @ 2012-01-06 14:07 UTC (permalink / raw)
To: linux-mm
Cc: KAMEZAWA Hiroyuki, David Rientjes, Andrew Morton, Hillf Danton, LKML
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)) {
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mm: vmscan: recompute page status when putting back 2012-01-06 14:07 [PATCH] mm: vmscan: recompute page status when putting back Hillf Danton @ 2012-01-09 16:49 ` KOSAKI Motohiro 2012-01-10 3:44 ` KAMEZAWA Hiroyuki 1 sibling, 0 replies; 4+ messages in thread From: KOSAKI Motohiro @ 2012-01-09 16:49 UTC (permalink / raw) To: Hillf Danton Cc: linux-mm, KAMEZAWA Hiroyuki, David Rientjes, Andrew Morton, LKML > 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); When and How do this race happen? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: vmscan: recompute page status when putting back 2012-01-06 14:07 [PATCH] mm: vmscan: recompute page status when putting back Hillf Danton 2012-01-09 16:49 ` KOSAKI Motohiro @ 2012-01-10 3:44 ` KAMEZAWA Hiroyuki 2012-01-10 14:32 ` Hillf Danton 1 sibling, 1 reply; 4+ messages in thread From: KAMEZAWA Hiroyuki @ 2012-01-10 3:44 UTC (permalink / raw) To: Hillf Danton; +Cc: linux-mm, David Rientjes, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: vmscan: recompute page status when putting back 2012-01-10 3:44 ` KAMEZAWA Hiroyuki @ 2012-01-10 14:32 ` Hillf Danton 0 siblings, 0 replies; 4+ messages in thread From: Hillf Danton @ 2012-01-10 14:32 UTC (permalink / raw) To: KAMEZAWA Hiroyuki; +Cc: linux-mm, David Rientjes, Andrew Morton, LKML On Tue, Jan 10, 2012 at 11:44 AM, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote: > 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 ? > Hi Kame Thanks for reviewing my work. With focus on the case that redo occurs, the patch was prepared based on the assumption that any isolated page could be processed by the function. If redo does occur, though unlikely, there are two rounds of isolation+putback or more for the given page. As shown by my workout of page status, difference exists in the two cases. ============================================================== without redo with redo ============================================================== active true 50% true false 50% false 100% ============================================================== was_unevictable true 50% true 100% false 50% false ============================================================== And the case with redo could be covered by the case without redo, so there is no corruption of VM events. Hillf ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-10 14:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-06 14:07 [PATCH] mm: vmscan: recompute page status when putting back Hillf Danton 2012-01-09 16:49 ` KOSAKI Motohiro 2012-01-10 3:44 ` KAMEZAWA Hiroyuki 2012-01-10 14:32 ` Hillf Danton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).