Re: Found the commit that causes the OOMs

[Minchan Kim <minchan.kim@gmail.com>]

Hi, Hannes.

On Sat, Jun 27, 2009 at 9:54 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
> On Sat, Jun 27, 2009 at 08:12:49AM +0100, David Howells wrote:
>>
>> I've managed to bisect things to find the commit that causes the OOMs.  It's:
>>
>>       commit 69c854817566db82c362797b4a6521d0b00fe1d8
>>       Author: MinChan Kim <minchan.kim@gmail.com>
>>       Date:   Tue Jun 16 15:32:44 2009 -0700
>>
>>           vmscan: prevent shrinking of active anon lru list in case of no swap space V3
>>
>>           shrink_zone() can deactivate active anon pages even if we don't have a
>>           swap device.  Many embedded products don't have a swap device.  So the
>>           deactivation of anon pages is unnecessary.
>>
>>           This patch prevents unnecessary deactivation of anon lru pages.  But, it
>>           don't prevent aging of anon pages to swap out.
>>
>>           Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>>           Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>           Cc: Johannes Weiner <hannes@cmpxchg.org>
>>           Acked-by: Rik van Riel <riel@redhat.com>
>>           Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>           Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> This exhibits the problem.  The previous commit:
>>
>>       commit 35282a2de4e5e4e173ab61aa9d7015886021a821
>>       Author: Brice Goglin <Brice.Goglin@ens-lyon.org>
>>       Date:   Tue Jun 16 15:32:43 2009 -0700
>>
>>           migration: only migrate_prep() once per move_pages()
>>
>> survives 16 iterations of the LTP syscall testsuite without exhibiting the
>> problem.
>
> Here is the patch in question:
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7592d8e..879d034 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1570,7 +1570,7 @@ static void shrink_zone(int priority, struct zone *zone,
>         * Even if we did not try to evict anon pages at all, we want to
>         * rebalance the anon lru active/inactive ratio.
>         */
> -       if (inactive_anon_is_low(zone, sc))
> +       if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>
>        throttle_vm_writeout(sc->gfp_mask);
>
> When this was discussed, I think we missed that nr_swap_pages can
> actually get zero on swap systems as well and this should have been
> total_swap_pages - otherwise we also stop balancing the two anon lists
> when swap is _full_ which was not the intention of this change at all.

At that time we considered it so that we didn't prevent anon list
aging for background reclaim.
Do you think it is not enough ?



-- 
Kinds regards,
Minchan Kim