[PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[Hugh Dickins]

mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
quicker than last time: one bug fixed but another bug introduced.
vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
forgot to add NR_LRU_BASE to lru index to make zone_page_state index.

Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---

 mm/vmscan.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- mmotm/mm/vmscan.c	2009-08-28 10:07:57.000000000 +0100
+++ linux/mm/vmscan.c	2009-08-28 18:30:33.000000000 +0100
@@ -1381,8 +1381,10 @@ static void shrink_active_list(unsigned
 	reclaim_stat->recent_rotated[file] += nr_rotated;
 	__count_vm_events(PGDEACTIVATE, nr_deactivated);
 	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
-	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
-	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
+	__mod_zone_page_state(zone, NR_ACTIVE_ANON + file * LRU_FILE,
+							nr_rotated);
+	__mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
+							nr_deactivated);
 	spin_unlock_irq(&zone->lru_lock);
 }
 

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[Rik van Riel]

Hugh Dickins wrote:
> mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
> quicker than last time: one bug fixed but another bug introduced.
> vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
> forgot to add NR_LRU_BASE to lru index to make zone_page_state index.
> 
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[Minchan Kim]

On Sat, Aug 29, 2009 at 4:39 AM, Hugh Dickins<hugh.dickins@tiscali.co.uk> wrote:
> mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
> quicker than last time: one bug fixed but another bug introduced.
> vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
> forgot to add NR_LRU_BASE to lru index to make zone_page_state index.
>
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

-- 
Kind regards,
Minchan Kim

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[KOSAKI Motohiro]

Hi Hugh

2009/8/29 Hugh Dickins <hugh.dickins@tiscali.co.uk>:
> mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
> quicker than last time: one bug fixed but another bug introduced.
> vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
> forgot to add NR_LRU_BASE to lru index to make zone_page_state index.
>
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

Can I use your test case?
Currently LRU_BASE is 0. it mean

LRU_BASE == NR_INACTIVE_ANON == 0
LRU_ACTIVE == NR_ACTIVE_ANON == 1

Therefore, I doubt there are another issue in current mmotm.
Can I join your strange oom fixing works?


> ---
>
>  mm/vmscan.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --- mmotm/mm/vmscan.c   2009-08-28 10:07:57.000000000 +0100
> +++ linux/mm/vmscan.c   2009-08-28 18:30:33.000000000 +0100
> @@ -1381,8 +1381,10 @@ static void shrink_active_list(unsigned
>        reclaim_stat->recent_rotated[file] += nr_rotated;
>        __count_vm_events(PGDEACTIVATE, nr_deactivated);
>        __mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> -       __mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
> -       __mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> +       __mod_zone_page_state(zone, NR_ACTIVE_ANON + file * LRU_FILE,
> +                                                       nr_rotated);
> +       __mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
> +                                                       nr_deactivated);
>        spin_unlock_irq(&zone->lru_lock);
>  }
>
>
> --
> 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/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[Johannes Weiner]

On Sat, Aug 29, 2009 at 07:00:47PM +0900, KOSAKI Motohiro wrote:
> Hi Hugh
> 
> 2009/8/29 Hugh Dickins <hugh.dickins@tiscali.co.uk>:
> > mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
> > quicker than last time: one bug fixed but another bug introduced.
> > vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
> > forgot to add NR_LRU_BASE to lru index to make zone_page_state index.
> >
> > Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> 
> Can I use your test case?
> Currently LRU_BASE is 0. it mean
> 
> LRU_BASE == NR_INACTIVE_ANON == 0
> LRU_ACTIVE == NR_ACTIVE_ANON == 1

The zone counters are

	NR_FREE_PAGES = 0
	NR_INACTIVE_ANON = NR_LRU_BASE = 1
	NR_ACTIVE_ANON = 2
	...,

and NR_LRU_BASE is the offset of the LRU items within the zone stat
items.  You missed this offset, so accounting to LRU_BASE + 0 *
LRU_FILE actually accounts to NR_FREE_PAGES, not to NR_INACTIVE_ANON.

I get the feeling we should make this thing more robust...

	Hannes

> Therefore, I doubt there are another issue in current mmotm.
> Can I join your strange oom fixing works?
> 
> 
> > ---
> >
> > A mm/vmscan.c | A  A 6 ++++--
> > A 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > --- mmotm/mm/vmscan.c A  2009-08-28 10:07:57.000000000 +0100
> > +++ linux/mm/vmscan.c A  2009-08-28 18:30:33.000000000 +0100
> > @@ -1381,8 +1381,10 @@ static void shrink_active_list(unsigned
> > A  A  A  A reclaim_stat->recent_rotated[file] += nr_rotated;
> > A  A  A  A __count_vm_events(PGDEACTIVATE, nr_deactivated);
> > A  A  A  A __mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> > - A  A  A  __mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
> > - A  A  A  __mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> > + A  A  A  __mod_zone_page_state(zone, NR_ACTIVE_ANON + file * LRU_FILE,
> > + A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  nr_rotated);
> > + A  A  A  __mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
> > + A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  nr_deactivated);
> > A  A  A  A spin_unlock_irq(&zone->lru_lock);
> > A }

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix

[KOSAKI Motohiro]

2009/8/29 Johannes Weiner <hannes@cmpxchg.org>:
> On Sat, Aug 29, 2009 at 07:00:47PM +0900, KOSAKI Motohiro wrote:
>> Hi Hugh
>>
>> 2009/8/29 Hugh Dickins <hugh.dickins@tiscali.co.uk>:
>> > mmotm 2009-08-27-16-51 lets the OOM killer loose on my loads even
>> > quicker than last time: one bug fixed but another bug introduced.
>> > vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
>> > forgot to add NR_LRU_BASE to lru index to make zone_page_state index.
>> >
>> > Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
>>
>> Can I use your test case?
>> Currently LRU_BASE is 0. it mean
>>
>> LRU_BASE == NR_INACTIVE_ANON == 0
>> LRU_ACTIVE == NR_ACTIVE_ANON == 1
>
> The zone counters are
>
>        NR_FREE_PAGES = 0
>        NR_INACTIVE_ANON = NR_LRU_BASE = 1
>        NR_ACTIVE_ANON = 2
>        ...,
>
> and NR_LRU_BASE is the offset of the LRU items within the zone stat
> items.  You missed this offset, so accounting to LRU_BASE + 0 *
> LRU_FILE actually accounts to NR_FREE_PAGES, not to NR_INACTIVE_ANON.

/me slapt self. thank you correct me ;)


> I get the feeling we should make this thing more robust...

I agree your perfectly.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix2

[Hugh Dickins]

A second fix to the ill-starred
vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
which, once corrected to update the right counters by the first fix,
builds up absurdly large Active counts in /proc/meminfo.

nr_rotated is not the number of pages added back to the active list
(maybe it once was, maybe it should be again: but if so that's not
any business for a code rearrangement patch).  shrink_active_list()
needs to keep a separate nr_reactivated count of those.

Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---
Or... revert the offending patch and its first fix.

 mm/vmscan.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- mmotm/mm/vmscan.c	2009-08-28 18:30:33.000000000 +0100
+++ linux/mm/vmscan.c	2009-09-02 01:28:34.000000000 +0100
@@ -1306,6 +1306,7 @@ static void shrink_active_list(unsigned
 	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(zone, sc);
 	unsigned long nr_rotated = 0;
 	unsigned long nr_deactivated = 0;
+	unsigned long nr_reactivated = 0;
 
 	lru_add_drain();
 	spin_lock_irq(&zone->lru_lock);
@@ -1354,6 +1355,7 @@ static void shrink_active_list(unsigned
 			 */
 			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
 				list_add(&page->lru, &l_active);
+				nr_reactivated++;
 				continue;
 			}
 		}
@@ -1382,7 +1384,7 @@ static void shrink_active_list(unsigned
 	__count_vm_events(PGDEACTIVATE, nr_deactivated);
 	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
 	__mod_zone_page_state(zone, NR_ACTIVE_ANON + file * LRU_FILE,
-							nr_rotated);
+							nr_reactivated);
 	__mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
 							nr_deactivated);
 	spin_unlock_irq(&zone->lru_lock);

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm] vmscan move pgdeactivate modification to shrink_active_list fix2

[KOSAKI Motohiro]

> A second fix to the ill-starred
> vmscan-move-pgdeactivate-modification-to-shrink_active_list.patch
> which, once corrected to update the right counters by the first fix,
> builds up absurdly large Active counts in /proc/meminfo.
> 
> nr_rotated is not the number of pages added back to the active list
> (maybe it once was, maybe it should be again: but if so that's not
> any business for a code rearrangement patch).  shrink_active_list()
> needs to keep a separate nr_reactivated count of those.
> 
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> ---
> Or... revert the offending patch and its first fix.

Yes, The original patch author should be layoff :(

Andrew, can you please drop the patch?
I need to clean my brain and the patch need proper additional test.

I plan to resubmit it at next -rc2 or -rc3.


I'm sorry.


--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>