[PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[MinChan Kim]

Changelog since V3
 o Remove can_reclaim_anon. 
 o Add nr_swap_page > 0 in only shrink_zone - By Rik's advise.
 o Change patch description.

Changelog since V2
 o Add new function - can_reclaim_anon : it tests anon_list can be reclaim.

Changelog since V1
 o Use nr_swap_pages <= 0 in shrink_active_list to prevent scanning  of active anon list.

Now 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.

Thanks for good review. Rik and Kosaki.  

Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Rik van Riel <riel@redhat.com>

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

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2f9d555..621708f 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1577,7 +1577,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);
-- 
1.5.4.3


-- 
Kinds Regards,
Minchan Kim 

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[KOSAKI Motohiro]

>  mm/vmscan.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2f9d555..621708f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1577,7 +1577,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);


       if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))

is better?
compiler can't swap evaluate order around &&.

then,

    if ( 0 && inactive_anon_is_low(zone, sc))

and

    if (inactive_anon_is_low(zone, sc) && 0)

are different.

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Minchan Kim]

On Thu, May 14, 2009 at 11:27 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>>  mm/vmscan.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 2f9d555..621708f 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1577,7 +1577,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);
>
>
>       if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))
>
> is better?
> compiler can't swap evaluate order around &&.

If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned,
we don't have a concern.

-- 
Thanks,
Minchan Kim

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[KOSAKI Motohiro]

2009/5/14 Minchan Kim <barrioskmc@gmail.com>:
> On Thu, May 14, 2009 at 11:27 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
>>>  mm/vmscan.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 2f9d555..621708f 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -1577,7 +1577,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);
>>
>>
>>       if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))
>>
>> is better?
>> compiler can't swap evaluate order around &&.
>
> If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned,
> we don't have a concern.

ok. I ack this.

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Rik van Riel]

MinChan Kim wrote:

> This patch prevents unnecessary deactivation of anon lru pages.
> But, it don't prevent aging of anon pages to swap out.

> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1577,7 +1577,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);

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

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Johannes Weiner]

On Thu, May 14, 2009 at 11:39:49PM +0900, Minchan Kim wrote:
> On Thu, May 14, 2009 at 11:27 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >>  mm/vmscan.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >> index 2f9d555..621708f 100644
> >> --- a/mm/vmscan.c
> >> +++ b/mm/vmscan.c
> >> @@ -1577,7 +1577,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);
> >
> >
> >       if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))
> >
> > is better?
> > compiler can't swap evaluate order around &&.
> 
> If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned,
> we don't have a concern.

It can only optimize it away when the condition is a compile time
constant.

But inactive_anon_is_low() contains atomic operations which the
compiler is not allowed to drop and so the && semantics lead to

	atomic_read() && 0

emitting the read while still knowing the whole expression is 0 at
compile-time, optimizing away only the branch itself but leaving the
read in place!

Compared to

	0 && atomic_read()

where the && short-circuitry leads to atomic_read() not being
executed.  And since the 0 is a compile time constant, no code has to
be emitted for the read.

So KOSAKI-san's is right.  Your version results in bigger object code.

	Hannes

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Andrew Morton]

On Thu, 14 May 2009 23:15:55 +0900
MinChan Kim <minchan.kim@gmail.com> wrote:

> Now 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. 

Does shrink_list() need to scan the anon LRUs at all if there's no swap
online?

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Rik van Riel]

Andrew Morton wrote:
> On Thu, 14 May 2009 23:15:55 +0900
> MinChan Kim <minchan.kim@gmail.com> wrote:
> 
>> Now 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. 
> 
> Does shrink_list() need to scan the anon LRUs at all if there's no swap
> online?

It doesn't.  Get_scan_ratio() will return 0 as the
anon percentage if no swap is online.

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Minchan Kim]

On Fri, May 15, 2009 at 1:22 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Thu, May 14, 2009 at 11:39:49PM +0900, Minchan Kim wrote:
>> On Thu, May 14, 2009 at 11:27 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> >>  mm/vmscan.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> >> index 2f9d555..621708f 100644
>> >> --- a/mm/vmscan.c
>> >> +++ b/mm/vmscan.c
>> >> @@ -1577,7 +1577,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);
>> >
>> >
>> >       if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))
>> >
>> > is better?
>> > compiler can't swap evaluate order around &&.
>>
>> If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned,
>> we don't have a concern.
>
> It can only optimize it away when the condition is a compile time
> constant.
>
> But inactive_anon_is_low() contains atomic operations which the
> compiler is not allowed to drop and so the && semantics lead to
>
>        atomic_read() && 0
>
> emitting the read while still knowing the whole expression is 0 at
> compile-time, optimizing away only the branch itself but leaving the
> read in place!
>
> Compared to
>
>        0 && atomic_read()
>
> where the && short-circuitry leads to atomic_read() not being
> executed.  And since the 0 is a compile time constant, no code has to
> be emitted for the read.
>
> So KOSAKI-san's is right.  Your version results in bigger object code.

You're right.  I realized it from you.
I will repost this.
Thanks for great review, Hannes :)

>        Hannes
>



-- 
Kinds regards,
Minchan Kim

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Wu Fengguang]

On Thu, May 14, 2009 at 11:15:55PM +0900, MinChan Kim wrote:
 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2f9d555..621708f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1577,7 +1577,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);

There's another "if (inactive_anon_is_low) shrink_active_list;"
occurrence to be fixed in balance_pgdat()? Otherwise:

Acked-by: Wu Fengguang <fengguang.wu@intel.com>

Re: [PATCH] mmtom: Prevent shrinking of active anon lru list in case of no swap space V3

[Minchan Kim]

On Mon, 18 May 2009 10:34:04 +0800
Wu Fengguang <fengguang.wu@intel.com> wrote:

> On Thu, May 14, 2009 at 11:15:55PM +0900, MinChan Kim wrote:
>  
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 2f9d555..621708f 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1577,7 +1577,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);
> 
> There's another "if (inactive_anon_is_low) shrink_active_list;"
> occurrence to be fixed in balance_pgdat()? Otherwise:

I add Rik's comment. Of course, I agree his opinion. 

"If we are close to running out of swap space, with
swapins freeing up swap space on a regular basis,
I believe we do want to do aging on the active
pages, just so we can pick a decent page to swap
out next time swap space becomes available."

> Acked-by: Wu Fengguang <fengguang.wu@intel.com>

Thanks for spending your time for my patch. Wu Fengguang :)

-- 
Kinds Regards
Minchan Kim