[PATCH] vmscan: prevent background aging of anon page in no swap system

[PATCH] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

Ying Han reported that backing aging of anon pages in no swap system
causes unnecessary TLB flush.

When I sent a patch(69c8548175), I wanted this patch but Rik pointed out
and allowed aging of anon pages to give a chance to promote from inactive
to active LRU.

It has a two problem.

1) non-swap system

Never make sense to age anon pages.

2) swap configured but still doesn't swapon

It doesn't make sense to age anon pages until swap-on time.
But it's arguable. If we have aged anon pages by swapon, VM have moved
anon pages from active to inactive. And in the time swapon by admin,
the VM can't reclaim hot pages so we can protect hot pages swapout.

But let's think about it. When does swap-on happen? It depends on admin.
we can't expect it. Nonetheless, we have done aging of anon pages to
protect hot pages swapout. It means we lost run time overhead when
below high watermark but gain hot page swap-[in/out] overhead when VM
decide swapout. Is it true? Let's think more detail.
We don't promote anon pages in case of non-swap system. So even though
VM does aging of anon pages, the pages would be in inactive LRU for a long
time. It means many of pages in there would mark access bit again. So access
bit hot/code separation would be pointless.

This patch prevents unnecessary anon pages demotion in not-swapon and
non-configured swap system. Of course, it could make side effect that
hot anon pages could swap out when admin does swap on.
But I think sooner or later it would be steady state. 
So it's not a big problem.
We could lose someting but gain more thing(TLB flush and unnecessary 
function call to demote anon pages). 

I used total_swap_pages because we want to age anon pages 
even though swap full happens.

Cc: Rik van Riel <riel@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Reported-by: Ying Han <yinghan@google.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/vmscan.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3109ff7..d8fd87d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2211,7 +2211,7 @@ loop_again:
 			 * Do some background aging of the anon list, to give
 			 * pages a chance to be referenced before reclaiming.
 			 */
-			if (inactive_anon_is_low(zone, &sc))
+			if (total_swap_pages > 0 && inactive_anon_is_low(zone, &sc))
 				shrink_active_list(SWAP_CLUSTER_MAX, zone,
 							&sc, priority, 0);
 
-- 
1.7.0.5

--
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] vmscan: prevent background aging of anon page in no swap system

[Rik van Riel]

On 08/29/2010 11:43 AM, Minchan Kim wrote:

> This patch prevents unnecessary anon pages demotion in not-swapon and
> non-configured swap system. Of course, it could make side effect that
> hot anon pages could swap out when admin does swap on.
> But I think sooner or later it would be steady state.
> So it's not a big problem.
> We could lose someting but gain more thing(TLB flush and unnecessary
> function call to demote anon pages).

A agree that is not a big worry.  I expect virtually all the
systems with swap space will do swapon at boot time.

> I used total_swap_pages because we want to age anon pages
> even though swap full happens.
>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> Cc: Johannes Weiner<hannes@cmpxchg.org>
> Reported-by: Ying Han<yinghan@google.com>
> Signed-off-by: Minchan Kim<minchan.kim@gmail.com>

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] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

On Sun, Aug 29, 2010 at 8:43 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> Ying Han reported that backing aging of anon pages in no swap system
> causes unnecessary TLB flush.
>
> When I sent a patch(69c8548175), I wanted this patch but Rik pointed out
> and allowed aging of anon pages to give a chance to promote from inactive
> to active LRU.
>
> It has a two problem.
>
> 1) non-swap system
>
> Never make sense to age anon pages.
>
> 2) swap configured but still doesn't swapon
>
> It doesn't make sense to age anon pages until swap-on time.
> But it's arguable. If we have aged anon pages by swapon, VM have moved
> anon pages from active to inactive. And in the time swapon by admin,
> the VM can't reclaim hot pages so we can protect hot pages swapout.
>
> But let's think about it. When does swap-on happen? It depends on admin.
> we can't expect it. Nonetheless, we have done aging of anon pages to
> protect hot pages swapout. It means we lost run time overhead when
> below high watermark but gain hot page swap-[in/out] overhead when VM
> decide swapout. Is it true? Let's think more detail.
> We don't promote anon pages in case of non-swap system. So even though
> VM does aging of anon pages, the pages would be in inactive LRU for a long
> time. It means many of pages in there would mark access bit again. So access
> bit hot/code separation would be pointless.
>
> This patch prevents unnecessary anon pages demotion in not-swapon and
> non-configured swap system. Of course, it could make side effect that
> hot anon pages could swap out when admin does swap on.
> But I think sooner or later it would be steady state.
> So it's not a big problem.
> We could lose someting but gain more thing(TLB flush and unnecessary
> function call to demote anon pages).
>
> I used total_swap_pages because we want to age anon pages
> even though swap full happens.
>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Reported-by: Ying Han <yinghan@google.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> ---
>  mm/vmscan.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3109ff7..d8fd87d 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2211,7 +2211,7 @@ loop_again:
>                         * Do some background aging of the anon list, to give
>                         * pages a chance to be referenced before reclaiming.
>                         */
> -                       if (inactive_anon_is_low(zone, &sc))
> +                       if (total_swap_pages > 0 && inactive_anon_is_low(zone, &sc))
>                                shrink_active_list(SWAP_CLUSTER_MAX, zone,
>                                                        &sc, priority, 0);
>
> --
> 1.7.0.5
>
>

There are few other places in vmscan where we check nr_swap_pages and
inactive_anon_is_low. Are we planning to change them to use
total_swap_pages
to be consistent ?

--Ying

--
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] vmscan: prevent background aging of anon page in no swap system

[Rik van Riel]

On 08/29/2010 01:45 PM, Ying Han wrote:

> There are few other places in vmscan where we check nr_swap_pages and
> inactive_anon_is_low. Are we planning to change them to use
> total_swap_pages
> to be consistent ?

If that makes sense, maybe the check can just be moved into
inactive_anon_is_low itself?

-- 
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] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:

> On 08/29/2010 01:45 PM, Ying Han wrote:
>
>  There are few other places in vmscan where we check nr_swap_pages and
>> inactive_anon_is_low. Are we planning to change them to use
>> total_swap_pages
>> to be consistent ?
>>
>
> If that makes sense, maybe the check can just be moved into
> inactive_anon_is_low itself?
>

That was the initial patch posted, instead we changed to use
total_swap_pages instead. How this patch looks:

@@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone *zone,
struct scan_control *sc)
 {
        int low;

+       if (total_swap_pages <= 0)
+               return 0;
+
        if (scanning_global_lru(sc))
                low = inactive_anon_is_low_global(zone);
        else
@@ -1856,7 +1859,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) && nr_swap_pages > 0)
+       if (inactive_anon_is_low(zone, sc))
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

        throttle_vm_writeout(sc->gfp_mask);

--Ying

>
> --
> All rights reversed
>

[-- Attachment #2: Type: text/html, Size: 2121 bytes --]

Re: [PATCH] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
> On 08/29/2010 01:45 PM, Ying Han wrote:
>
>> There are few other places in vmscan where we check nr_swap_pages and
>> inactive_anon_is_low. Are we planning to change them to use
>> total_swap_pages
>> to be consistent ?
>
> If that makes sense, maybe the check can just be moved into
> inactive_anon_is_low itself?

That was the initial patch posted, instead we changed to use
total_swap_pages instead. How this patch looks:

@@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
*zone, struct scan_control *sc)
 {
        int low;

+       if (total_swap_pages <= 0)
+               return 0;
+
        if (scanning_global_lru(sc))
                low = inactive_anon_is_low_global(zone);
        else
@@ -1856,7 +1859,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) && nr_swap_pages > 0)
+       if (inactive_anon_is_low(zone, sc))
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

        throttle_vm_writeout(sc->gfp_mask);

--Ying

>
> --
> 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] vmscan: prevent background aging of anon page in no swap system

[Rik van Riel]

On 08/29/2010 05:23 PM, Ying Han wrote:
> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel<riel@redhat.com>  wrote:
>> On 08/29/2010 01:45 PM, Ying Han wrote:
>>
>>> There are few other places in vmscan where we check nr_swap_pages and
>>> inactive_anon_is_low. Are we planning to change them to use
>>> total_swap_pages
>>> to be consistent ?
>>
>> If that makes sense, maybe the check can just be moved into
>> inactive_anon_is_low itself?
>
> That was the initial patch posted, instead we changed to use
> total_swap_pages instead. How this patch looks:

Looks good to me.  It could use a comment along the lines of:

	/*
	 * No sense scanning the anon lists if we have no swap space.
	 */

... and, of course, your signed-off-by :)

-- 
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

Hi Ying,

On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <yinghan@google.com> wrote:
> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
>> On 08/29/2010 01:45 PM, Ying Han wrote:
>>
>>> There are few other places in vmscan where we check nr_swap_pages and
>>> inactive_anon_is_low. Are we planning to change them to use
>>> total_swap_pages
>>> to be consistent ?
>>
>> If that makes sense, maybe the check can just be moved into
>> inactive_anon_is_low itself?
>
> That was the initial patch posted, instead we changed to use
> total_swap_pages instead. How this patch looks:
>
> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
> *zone, struct scan_control *sc)
>  {
>        int low;
>
> +       if (total_swap_pages <= 0)
> +               return 0;
> +
>        if (scanning_global_lru(sc))
>                low = inactive_anon_is_low_global(zone);
>        else
> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
> +       if (inactive_anon_is_low(zone, sc))
>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>
>        throttle_vm_writeout(sc->gfp_mask);
>
> --Ying
>
>>

I did it intentionally since inactive_anon_is_low have been used both
direct reclaim and background path. In this point, your patch could
make side effect in swap enabled system when swap is full.

I think we need aging in only background if system is swap full.
That's because if the swap space is full, we don't reclaim anon pages
in direct reclaim path with (nr_swap_pages < 0)  and even have been
not rebalance it until now.
I think direct reclaim path is important about latency as well as
reclaim's effectiveness.
So if you don't mind, I hope direct reclaim patch would be left just as it is.

-- 
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] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> Hi Ying,
>
> On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <yinghan@google.com> wrote:
>> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
>>> On 08/29/2010 01:45 PM, Ying Han wrote:
>>>
>>>> There are few other places in vmscan where we check nr_swap_pages and
>>>> inactive_anon_is_low. Are we planning to change them to use
>>>> total_swap_pages
>>>> to be consistent ?
>>>
>>> If that makes sense, maybe the check can just be moved into
>>> inactive_anon_is_low itself?
>>
>> That was the initial patch posted, instead we changed to use
>> total_swap_pages instead. How this patch looks:
>>
>> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
>> *zone, struct scan_control *sc)
>>  {
>>        int low;
>>
>> +       if (total_swap_pages <= 0)
>> +               return 0;
>> +
>>        if (scanning_global_lru(sc))
>>                low = inactive_anon_is_low_global(zone);
>>        else
>> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
>> +       if (inactive_anon_is_low(zone, sc))
>>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>>
>>        throttle_vm_writeout(sc->gfp_mask);
>>
>> --Ying
>>
>>>
>
> I did it intentionally since inactive_anon_is_low have been used both
> direct reclaim and background path. In this point, your patch could
> make side effect in swap enabled system when swap is full.
>
> I think we need aging in only background if system is swap full.
> That's because if the swap space is full, we don't reclaim anon pages
> in direct reclaim path with (nr_swap_pages < 0)  and even have been
> not rebalance it until now.
> I think direct reclaim path is important about latency as well as
> reclaim's effectiveness.
> So if you don't mind, I hope direct reclaim patch would be left just as it is.

Minchan, I would prefer to make kswapd as well as direct reclaim to be
consistent if possible.
They both try to reclaim pages when system is under memory pressure,
and also do not make
much sense to look at anon lru if no swap space available. Either
because of no swapon or run
out of swap space.

I think letting kswapd to age anon lru without free swap space is not
necessary neither. That leads
to my initial patch:

@@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
*zone, struct scan_control *sc)
 {
       int low;

+       if (nr_swap_pages <= 0)
+               return 0;
+
       if (scanning_global_lru(sc))
               low = inactive_anon_is_low_global(zone);
       else
@@ -1856,7 +1859,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) && nr_swap_pages > 0)
+       if (inactive_anon_is_low(zone, sc))
               shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

What do you think ?

--Ying
>
> --
> 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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Mon, Aug 30, 2010 at 2:40 PM, Ying Han <yinghan@google.com> wrote:
> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> Hi Ying,
>>
>> On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <yinghan@google.com> wrote:
>>> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
>>>> On 08/29/2010 01:45 PM, Ying Han wrote:
>>>>
>>>>> There are few other places in vmscan where we check nr_swap_pages and
>>>>> inactive_anon_is_low. Are we planning to change them to use
>>>>> total_swap_pages
>>>>> to be consistent ?
>>>>
>>>> If that makes sense, maybe the check can just be moved into
>>>> inactive_anon_is_low itself?
>>>
>>> That was the initial patch posted, instead we changed to use
>>> total_swap_pages instead. How this patch looks:
>>>
>>> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
>>> *zone, struct scan_control *sc)
>>>  {
>>>        int low;
>>>
>>> +       if (total_swap_pages <= 0)
>>> +               return 0;
>>> +
>>>        if (scanning_global_lru(sc))
>>>                low = inactive_anon_is_low_global(zone);
>>>        else
>>> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
>>> +       if (inactive_anon_is_low(zone, sc))
>>>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>>>
>>>        throttle_vm_writeout(sc->gfp_mask);
>>>
>>> --Ying
>>>
>>>>
>>
>> I did it intentionally since inactive_anon_is_low have been used both
>> direct reclaim and background path. In this point, your patch could
>> make side effect in swap enabled system when swap is full.
>>
>> I think we need aging in only background if system is swap full.
>> That's because if the swap space is full, we don't reclaim anon pages
>> in direct reclaim path with (nr_swap_pages < 0)  and even have been
>> not rebalance it until now.
>> I think direct reclaim path is important about latency as well as
>> reclaim's effectiveness.
>> So if you don't mind, I hope direct reclaim patch would be left just as it is.
>
> Minchan, I would prefer to make kswapd as well as direct reclaim to be
> consistent if possible.
> They both try to reclaim pages when system is under memory pressure,
> and also do not make
> much sense to look at anon lru if no swap space available. Either
> because of no swapon or run
> out of swap space.

In out of swap space, The few swap space would become more precious.
So I think we still need background aging to protect hot page swap out.
But I admit it's hard to measure it so I can't insist on.
But I wanted to maintain it as it is to avoid _unexpected_ side effect.

And your patch can't compile out inactive_anon_is_low call in non swap
configurable system. It makes unnecessary call. So I want to use
nr_swap_pages && inactive_anon_is_low.

For it, I sended following patch at last version

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 1b145e6..0b8a3ce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1747,7 +1747,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) && nr_swap_pages > 0)
+       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

        throttle_vm_writeout(sc->gfp_mask);

But Andrew merged middle version.
I will send this patch again.

Thanks.

-- 
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1b145e6..0b8a3ce 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1747,7 +1747,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) && nr_swap_pages > 0)
> +       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))

Sorry, I don't find any difference. What is your intention?


>                 shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
> 
>         throttle_vm_writeout(sc->gfp_mask);
> 
> But Andrew merged middle version.
> I will send this patch again.
> 
> Thanks.
> 
> -- 
> 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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

Hi, KOSAKI.

On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 1b145e6..0b8a3ce 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1747,7 +1747,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) && nr_swap_pages > 0)
>> +       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))
>
> Sorry, I don't find any difference. What is your intention?
>

My intention is that smart gcc can compile out inactive_anon_is_low
call in case of non swap configurable system.

-- 
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> Hi, KOSAKI.
> 
> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >> index 1b145e6..0b8a3ce 100644
> >> --- a/mm/vmscan.c
> >> +++ b/mm/vmscan.c
> >> @@ -1747,7 +1747,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) && nr_swap_pages > 0)
> >> +       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))
> >
> > Sorry, I don't find any difference. What is your intention?
> >
> 
> My intention is that smart gcc can compile out inactive_anon_is_low
> call in case of non swap configurable system.

Do you really check it on your gcc? nr_swap_pages is not file scope variable, it's
global variable. afaik, current gcc's link time optimization is not so cool.

Do you have a disassemble list?



--
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Tue, Aug 31, 2010 at 10:18 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> Hi, KOSAKI.
>>
>> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> >> index 1b145e6..0b8a3ce 100644
>> >> --- a/mm/vmscan.c
>> >> +++ b/mm/vmscan.c
>> >> @@ -1747,7 +1747,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) && nr_swap_pages > 0)
>> >> +       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))
>> >
>> > Sorry, I don't find any difference. What is your intention?
>> >
>>
>> My intention is that smart gcc can compile out inactive_anon_is_low
>> call in case of non swap configurable system.
>
> Do you really check it on your gcc? nr_swap_pages is not file scope variable, it's
> global variable. afaik, current gcc's link time optimization is not so cool.

#else /* CONFIG_SWAP */

#define nr_swap_pages                           0L
#define total_swap_pages                        0L
#define total_swapcache_pages                   0UL

>
> Do you have a disassemble list?
>

environment for test :
gcc : arm-none-linux-gnueabi-gcc (Sourcery G++ Lite 2009q3-67) 4.4.1
kernel : 2.6.28(for test, I used my working kernel version with my patch)
assembled function is shrink_zone.
(Please understand web gmail's contents mangling. Google guys! Please
repair for like me who can't use SMTP in company. :( )

1. swap configurable version

    1cd0:       e51b303c        ldr     r3, [fp, #-60]  ; 0x3c
    1cd4:       e3530000        cmp     r3, #0
    1cd8:       1affffd1        bne     1c24 <shrink_zone+0x23c>
    1cdc:       e5879004        str     r9, [r7, #4]
    1ce0:       e1a00008        mov     r0, r8
    1ce4:       e1a01007        mov     r1, r7
    1ce8:       e1a04008        mov     r4, r8
    1cec:       ebfff8eb        bl      a0 <inactive_anon_is_low>     <==
    1cf0:       e1a05007        mov     r5, r7
    1cf4:       e3500000        cmp     r0, #0
    1cf8:       0a000006        beq     1d18 <shrink_zone+0x330>
    1cfc:       e1a01008        mov     r1, r8
    1d00:       e1a03006        mov     r3, r6
    1d04:       e3a00020        mov     r0, #32
    1d08:       e1a02007        mov     r2, r7
    1d0c:       e3a0c000        mov     ip, #0
    1d10:       e58dc000        str     ip, [sp]
    1d14:       ebfffa98        bl      77c <shrink_active_list>
    1d18:       e5950008        ldr     r0, [r5, #8]
    1d1c:       ebfffffe        bl      0 <throttle_vm_writeout>
    1d20:       e24bd028        sub     sp, fp, #40     ; 0x28

2. non swap configurable version

    1994:       e3530000        cmp     r3, #0
    1998:       0a000003        beq     19ac <shrink_zone+0x170>
    199c:       e598300c        ldr     r3, [r8, #12]
    19a0:       e593300c        ldr     r3, [r3, #12]
    19a4:       e3130701        tst     r3, #262144     ; 0x40000
    19a8:       0a000008        beq     19d0 <shrink_zone+0x194>
    19ac:       e51b3044        ldr     r3, [fp, #-68]  ; 0x44
    19b0:       e3530000        cmp     r3, #0
    19b4:       1affffd7        bne     1918 <shrink_zone+0xdc>
    19b8:       e51b3038        ldr     r3, [fp, #-56]  ; 0x38
    19bc:       e3530000        cmp     r3, #0
    19c0:       1affffd4        bne     1918 <shrink_zone+0xdc>
    19c4:       e51b303c        ldr     r3, [fp, #-60]  ; 0x3c
    19c8:       e3530000        cmp     r3, #0
    19cc:       1affffd1        bne     1918 <shrink_zone+0xdc>
    19d0:       e586a004        str     sl, [r6, #4]
    19d4:       e1a04006        mov     r4, r6
    19d8:       e5960008        ldr     r0, [r6, #8]
    19dc:       ebfffffe        bl      0 <throttle_vm_writeout>
    19e0:       e24bd028        sub     sp, fp, #40     ; 0x28

-- 
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> On Tue, Aug 31, 2010 at 10:18 AM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> Hi, KOSAKI.
> >>
> >> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
> >> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >> >> index 1b145e6..0b8a3ce 100644
> >> >> --- a/mm/vmscan.c
> >> >> +++ b/mm/vmscan.c
> >> >> @@ -1747,7 +1747,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) && nr_swap_pages > 0)
> >> >> +       if (nr_swap_pges > 0 && inactive_anon_is_low(zone, sc))
> >> >
> >> > Sorry, I don't find any difference. What is your intention?
> >> >
> >>
> >> My intention is that smart gcc can compile out inactive_anon_is_low
> >> call in case of non swap configurable system.
> >
> > Do you really check it on your gcc? nr_swap_pages is not file scope variable, it's
> > global variable. afaik, current gcc's link time optimization is not so cool.
> 
> #else /* CONFIG_SWAP */
> 
> #define nr_swap_pages                           0L
> #define total_swap_pages                        0L
> #define total_swapcache_pages                   0UL

Ahh, I missed, sorry.


> > Do you have a disassemble list?
> >
> 
> environment for test :
> gcc : arm-none-linux-gnueabi-gcc (Sourcery G++ Lite 2009q3-67) 4.4.1
> kernel : 2.6.28(for test, I used my working kernel version with my patch)
> assembled function is shrink_zone.
> (Please understand web gmail's contents mangling. Google guys! Please
> repair for like me who can't use SMTP in company. :( )
> 
> 1. swap configurable version
> 
>     1cd0:       e51b303c        ldr     r3, [fp, #-60]  ; 0x3c
>     1cd4:       e3530000        cmp     r3, #0
>     1cd8:       1affffd1        bne     1c24 <shrink_zone+0x23c>
>     1cdc:       e5879004        str     r9, [r7, #4]
>     1ce0:       e1a00008        mov     r0, r8
>     1ce4:       e1a01007        mov     r1, r7
>     1ce8:       e1a04008        mov     r4, r8
>     1cec:       ebfff8eb        bl      a0 <inactive_anon_is_low>     <==
>     1cf0:       e1a05007        mov     r5, r7
>     1cf4:       e3500000        cmp     r0, #0
>     1cf8:       0a000006        beq     1d18 <shrink_zone+0x330>
>     1cfc:       e1a01008        mov     r1, r8
>     1d00:       e1a03006        mov     r3, r6
>     1d04:       e3a00020        mov     r0, #32
>     1d08:       e1a02007        mov     r2, r7
>     1d0c:       e3a0c000        mov     ip, #0
>     1d10:       e58dc000        str     ip, [sp]
>     1d14:       ebfffa98        bl      77c <shrink_active_list>
>     1d18:       e5950008        ldr     r0, [r5, #8]
>     1d1c:       ebfffffe        bl      0 <throttle_vm_writeout>
>     1d20:       e24bd028        sub     sp, fp, #40     ; 0x28
> 
> 2. non swap configurable version
> 
>     1994:       e3530000        cmp     r3, #0
>     1998:       0a000003        beq     19ac <shrink_zone+0x170>
>     199c:       e598300c        ldr     r3, [r8, #12]
>     19a0:       e593300c        ldr     r3, [r3, #12]
>     19a4:       e3130701        tst     r3, #262144     ; 0x40000
>     19a8:       0a000008        beq     19d0 <shrink_zone+0x194>
>     19ac:       e51b3044        ldr     r3, [fp, #-68]  ; 0x44
>     19b0:       e3530000        cmp     r3, #0
>     19b4:       1affffd7        bne     1918 <shrink_zone+0xdc>
>     19b8:       e51b3038        ldr     r3, [fp, #-56]  ; 0x38
>     19bc:       e3530000        cmp     r3, #0
>     19c0:       1affffd4        bne     1918 <shrink_zone+0xdc>
>     19c4:       e51b303c        ldr     r3, [fp, #-60]  ; 0x3c
>     19c8:       e3530000        cmp     r3, #0
>     19cc:       1affffd1        bne     1918 <shrink_zone+0xdc>
>     19d0:       e586a004        str     sl, [r6, #4]
>     19d4:       e1a04006        mov     r4, r6
>     19d8:       e5960008        ldr     r0, [r6, #8]
>     19dc:       ebfffffe        bl      0 <throttle_vm_writeout>
>     19e0:       e24bd028        sub     sp, fp, #40     ; 0x28

Thanks,  I'm convinced. 


--
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > Hi Ying,
> >
> > On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <yinghan@google.com> wrote:
> >> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
> >>> On 08/29/2010 01:45 PM, Ying Han wrote:
> >>>
> >>>> There are few other places in vmscan where we check nr_swap_pages and
> >>>> inactive_anon_is_low. Are we planning to change them to use
> >>>> total_swap_pages
> >>>> to be consistent ?
> >>>
> >>> If that makes sense, maybe the check can just be moved into
> >>> inactive_anon_is_low itself?
> >>
> >> That was the initial patch posted, instead we changed to use
> >> total_swap_pages instead. How this patch looks:
> >>
> >> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
> >> *zone, struct scan_control *sc)
> >>  {
> >>        int low;
> >>
> >> +       if (total_swap_pages <= 0)
> >> +               return 0;
> >> +
> >>        if (scanning_global_lru(sc))
> >>                low = inactive_anon_is_low_global(zone);
> >>        else
> >> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
> >> +       if (inactive_anon_is_low(zone, sc))
> >>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
> >>
> >>        throttle_vm_writeout(sc->gfp_mask);
> >>
> >> --Ying
> >>
> >>>
> >
> > I did it intentionally since inactive_anon_is_low have been used both
> > direct reclaim and background path. In this point, your patch could
> > make side effect in swap enabled system when swap is full.
> >
> > I think we need aging in only background if system is swap full.
> > That's because if the swap space is full, we don't reclaim anon pages
> > in direct reclaim path with (nr_swap_pages < 0)  and even have been
> > not rebalance it until now.
> > I think direct reclaim path is important about latency as well as
> > reclaim's effectiveness.
> > So if you don't mind, I hope direct reclaim patch would be left just as it is.
> 
> Minchan, I would prefer to make kswapd as well as direct reclaim to be
> consistent if possible.
> They both try to reclaim pages when system is under memory pressure,
> and also do not make
> much sense to look at anon lru if no swap space available. Either
> because of no swapon or run
> out of swap space.
> 
> I think letting kswapd to age anon lru without free swap space is not
> necessary neither. That leads
> to my initial patch:
> 
> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
> *zone, struct scan_control *sc)
>  {
>        int low;
> 
> +       if (nr_swap_pages <= 0)
> +               return 0;
> +
>        if (scanning_global_lru(sc))
>                low = inactive_anon_is_low_global(zone);
>        else
> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
> +       if (inactive_anon_is_low(zone, sc))
>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
> 
> What do you think ?

Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


I think both Ying's and Minchan's opnion are right and makes sense.  however I _personally_
like Ying version because 1) this version is simpler 2) swap full is very rarely event 3)
no swap mounting is very common on HPC. so this version could have a chance to 
improvement hpc workload too.

In the other word, both avoiding unnecessary TLB flush and keeping proper page aging are
performance matter. so when we are talking performance, we always need to think frequency
of the event.

Anyway I'm very glad minchan who embedded developer pay attention server workload
carefully. Very thanks.




--
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > Hi Ying,
>> >
>> > On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <yinghan@google.com> wrote:
>> >> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <riel@redhat.com> wrote:
>> >>> On 08/29/2010 01:45 PM, Ying Han wrote:
>> >>>
>> >>>> There are few other places in vmscan where we check nr_swap_pages and
>> >>>> inactive_anon_is_low. Are we planning to change them to use
>> >>>> total_swap_pages
>> >>>> to be consistent ?
>> >>>
>> >>> If that makes sense, maybe the check can just be moved into
>> >>> inactive_anon_is_low itself?
>> >>
>> >> That was the initial patch posted, instead we changed to use
>> >> total_swap_pages instead. How this patch looks:
>> >>
>> >> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
>> >> *zone, struct scan_control *sc)
>> >>  {
>> >>        int low;
>> >>
>> >> +       if (total_swap_pages <= 0)
>> >> +               return 0;
>> >> +
>> >>        if (scanning_global_lru(sc))
>> >>                low = inactive_anon_is_low_global(zone);
>> >>        else
>> >> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
>> >> +       if (inactive_anon_is_low(zone, sc))
>> >>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>> >>
>> >>        throttle_vm_writeout(sc->gfp_mask);
>> >>
>> >> --Ying
>> >>
>> >>>
>> >
>> > I did it intentionally since inactive_anon_is_low have been used both
>> > direct reclaim and background path. In this point, your patch could
>> > make side effect in swap enabled system when swap is full.
>> >
>> > I think we need aging in only background if system is swap full.
>> > That's because if the swap space is full, we don't reclaim anon pages
>> > in direct reclaim path with (nr_swap_pages < 0)  and even have been
>> > not rebalance it until now.
>> > I think direct reclaim path is important about latency as well as
>> > reclaim's effectiveness.
>> > So if you don't mind, I hope direct reclaim patch would be left just as it is.
>>
>> Minchan, I would prefer to make kswapd as well as direct reclaim to be
>> consistent if possible.
>> They both try to reclaim pages when system is under memory pressure,
>> and also do not make
>> much sense to look at anon lru if no swap space available. Either
>> because of no swapon or run
>> out of swap space.
>>
>> I think letting kswapd to age anon lru without free swap space is not
>> necessary neither. That leads
>> to my initial patch:
>>
>> @@ -1605,6 +1605,9 @@ static int inactive_anon_is_low(struct zone
>> *zone, struct scan_control *sc)
>>  {
>>        int low;
>>
>> +       if (nr_swap_pages <= 0)
>> +               return 0;
>> +
>>        if (scanning_global_lru(sc))
>>                low = inactive_anon_is_low_global(zone);
>>        else
>> @@ -1856,7 +1859,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) && nr_swap_pages > 0)
>> +       if (inactive_anon_is_low(zone, sc))
>>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>>
>> What do you think ?
>
> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>
>
> I think both Ying's and Minchan's opnion are right and makes sense.  however I _personally_
> like Ying version because 1) this version is simpler 2) swap full is very rarely event 3)
> no swap mounting is very common on HPC. so this version could have a chance to
> improvement hpc workload too.

I agree.

>
> In the other word, both avoiding unnecessary TLB flush and keeping proper page aging are
> performance matter. so when we are talking performance, we always need to think frequency
> of the event.

Ying's one and mine both has a same effect.
Only difference happens swap is full. My version maintains old
behavior but Ying's one changes the behavior. I admit swap full is
rare event but I hoped not changed old behavior if we doesn't find any
problem.
If kswapd does aging when swap full happens, is it a problem?
We have been used to it from 2.6.28.

If we regard a code consistency is more important than _unexpected_
result, Okay. I don't mind it. :)
But at least we should do more thing to make the patch to compile out
for non-swap configurable system.


>
> Anyway I'm very glad minchan who embedded developer pay attention server workload
> carefully. Very thanks.
>

Thanks for the good comment. KOSAKI. :)
-- 
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> > I think both Ying's and Minchan's opnion are right and makes sense.  however I _personally_
> > like Ying version because 1) this version is simpler 2) swap full is very rarely event 3)
> > no swap mounting is very common on HPC. so this version could have a chance to
> > improvement hpc workload too.
> 
> I agree.
> 
> >
> > In the other word, both avoiding unnecessary TLB flush and keeping proper page aging are
> > performance matter. so when we are talking performance, we always need to think frequency
> > of the event.
> 
> Ying's one and mine both has a same effect.
> Only difference happens swap is full. My version maintains old
> behavior but Ying's one changes the behavior. I admit swap full is
> rare event but I hoped not changed old behavior if we doesn't find any
> problem.
> If kswapd does aging when swap full happens, is it a problem?
> We have been used to it from 2.6.28.
> 
> If we regard a code consistency is more important than _unexpected_
> result, Okay. I don't mind it. :)

To be honest, I don't mind the difference between you and Ying's version. because
_practically_ swap full occur mean the application has a bug. so, proper page aging
doesn't help so much. That's the reason why I said I prefer simper. I don't have 
strong opinion. I think it's not big matter.


> But at least we should do more thing to make the patch to compile out
> for non-swap configurable system.

Yes, It makes embedded happy :)



--
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Tue, Aug 31, 2010 at 10:38 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> > I think both Ying's and Minchan's opnion are right and makes sense.  however I _personally_
>> > like Ying version because 1) this version is simpler 2) swap full is very rarely event 3)
>> > no swap mounting is very common on HPC. so this version could have a chance to
>> > improvement hpc workload too.
>>
>> I agree.
>>
>> >
>> > In the other word, both avoiding unnecessary TLB flush and keeping proper page aging are
>> > performance matter. so when we are talking performance, we always need to think frequency
>> > of the event.
>>
>> Ying's one and mine both has a same effect.
>> Only difference happens swap is full. My version maintains old
>> behavior but Ying's one changes the behavior. I admit swap full is
>> rare event but I hoped not changed old behavior if we doesn't find any
>> problem.
>> If kswapd does aging when swap full happens, is it a problem?
>> We have been used to it from 2.6.28.
>>
>> If we regard a code consistency is more important than _unexpected_
>> result, Okay. I don't mind it. :)
>
> To be honest, I don't mind the difference between you and Ying's version. because
> _practically_ swap full occur mean the application has a bug. so, proper page aging
> doesn't help so much. That's the reason why I said I prefer simper. I don't have
> strong opinion. I think it's not big matter.
>
>
>> But at least we should do more thing to make the patch to compile out
>> for non-swap configurable system.
>
> Yes, It makes embedded happy :)
>
>

How about this?

(Not formal patch. If we agree, I will post it later when I have a SMTP).


Signed-off-by: Ying Han <yinghan@google.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3109ff7..c3c44a8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1579,7 +1579,7 @@ static void shrink_active_list(unsigned long
nr_pages, struct zone *zone,
        __mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
        spin_unlock_irq(&zone->lru_lock);
 }
-
+#if CONFIG_SWAP
 static int inactive_anon_is_low_global(struct zone *zone)
 {
        unsigned long active, inactive;
@@ -1605,12 +1605,21 @@ static int inactive_anon_is_low(struct zone
*zone, struct scan_control *sc)
 {
        int low;

+       if (nr_swap_pages)
+               return 0;
+
        if (scanning_global_lru(sc))
                low = inactive_anon_is_low_global(zone);
        else
                low = mem_cgroup_inactive_anon_is_low(sc->mem_cgroup);
        return low;
 }
+#else
+static inline int inactive_anon_is_low(struct zone *zone, struct
scan_control *sc)
+{
+       return 0;
+}
+#endif

 static int inactive_file_is_low_global(struct zone *zone)
 {
@@ -1856,7 +1865,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) && nr_swap_pages > 0)
+       if (inactive_anon_is_low(zone, sc))
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

        throttle_vm_writeout(sc->gfp_mask);


-- 
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] vmscan: prevent background aging of anon page in no swap system

[KOSAKI Motohiro]

> How about this?
> 
> (Not formal patch. If we agree, I will post it later when I have a SMTP).
> 
> 
> Signed-off-by: Ying Han <yinghan@google.com>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3109ff7..c3c44a8 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1579,7 +1579,7 @@ static void shrink_active_list(unsigned long
> nr_pages, struct zone *zone,
>         __mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
>         spin_unlock_irq(&zone->lru_lock);
>  }
> -
> +#if CONFIG_SWAP
>  static int inactive_anon_is_low_global(struct zone *zone)
>  {
>         unsigned long active, inactive;
> @@ -1605,12 +1605,21 @@ static int inactive_anon_is_low(struct zone
> *zone, struct scan_control *sc)
>  {
>         int low;
> 
> +       if (nr_swap_pages)
> +               return 0;

!nr_swap_pages ?



> +
>         if (scanning_global_lru(sc))
>                 low = inactive_anon_is_low_global(zone);
>         else
>                 low = mem_cgroup_inactive_anon_is_low(sc->mem_cgroup);
>         return low;
>  }
> +#else
> +static inline int inactive_anon_is_low(struct zone *zone, struct
> scan_control *sc)
> +{
> +       return 0;
> +}
> +#endif

Yup. I prefer this explicit #ifdef :)



--
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Tue, Aug 31, 2010 at 11:09 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> How about this?
>>
>> (Not formal patch. If we agree, I will post it later when I have a SMTP)

Re: [PATCH] vmscan: prevent background aging of anon page in no swap system

[Rik van Riel]

On 08/30/2010 09:23 PM, Minchan Kim wrote:

> Ying's one and mine both has a same effect.
> Only difference happens swap is full. My version maintains old
> behavior but Ying's one changes the behavior. I admit swap full is
> rare event but I hoped not changed old behavior if we doesn't find any
> problem.
> If kswapd does aging when swap full happens, is it a problem?

It may be a good thing, since swap will often be freed again
(when something is swapped in, or exits).

Having some more anonymous pages sit on the inactive list
gives them a chance to get used again, potentially giving
us a better chance of preserving the working set when swap
is full or near full a lot of the time.

-- 
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] vmscan: prevent background aging of anon page in no swap system

[Minchan Kim]

On Tue, Aug 31, 2010 at 11:30 AM, Rik van Riel <riel@redhat.com> wrote:
> On 08/30/2010 09:23 PM, Minchan Kim wrote:
>
>> Ying's one and mine both has a same effect.
>> Only difference happens swap is full. My version maintains old
>> behavior but Ying's one changes the behavior. I admit swap full is
>> rare event but I hoped not changed old behavior if we doesn't find any
>> problem.
>> If kswapd does aging when swap full happens, is it a problem?
>
> It may be a good thing, since swap will often be freed again
> (when something is swapped in, or exits).
>
> Having some more anonymous pages sit on the inactive list
> gives them a chance to get used again, potentially giving
> us a better chance of preserving the working set when swap
> is full or near full a lot of the time.

Do you mean we would be better to do background aging when swap is full?
I wanted it. So I used total_swap_pages to protect working set when
swap is full.
But Ying and KOSAKI's don't like it since it makes code inconsistent
or not simply.
And I agree it's rare event as KOSAKI mentioned.

Hmm... What do you think about it?

If you don't mind, I will resend latest version(use nr_swap_page usage
and compile out inactive_anon_is_low in case of !CONFIG_SWAP).


-- 
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] vmscan: prevent background aging of anon page in no swap system

[Andrew Morton]

On Mon, 30 Aug 2010 00:43:48 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:

> Ying Han reported that backing aging of anon pages in no swap system
> causes unnecessary TLB flush.
> 
> When I sent a patch(69c8548175), I wanted this patch but Rik pointed out
> and allowed aging of anon pages to give a chance to promote from inactive
> to active LRU.
> 
> It has a two problem.
> 
> 1) non-swap system
> 
> Never make sense to age anon pages.
> 
> 2) swap configured but still doesn't swapon
> 
> It doesn't make sense to age anon pages until swap-on time.
> But it's arguable. If we have aged anon pages by swapon, VM have moved
> anon pages from active to inactive. And in the time swapon by admin,
> the VM can't reclaim hot pages so we can protect hot pages swapout.
> 
> But let's think about it. When does swap-on happen? It depends on admin.
> we can't expect it. Nonetheless, we have done aging of anon pages to
> protect hot pages swapout. It means we lost run time overhead when
> below high watermark but gain hot page swap-[in/out] overhead when VM
> decide swapout. Is it true? Let's think more detail.
> We don't promote anon pages in case of non-swap system. So even though
> VM does aging of anon pages, the pages would be in inactive LRU for a long
> time. It means many of pages in there would mark access bit again. So access
> bit hot/code separation would be pointless.
> 
> This patch prevents unnecessary anon pages demotion in not-swapon and
> non-configured swap system. Of course, it could make side effect that
> hot anon pages could swap out when admin does swap on.
> But I think sooner or later it would be steady state. 
> So it's not a big problem.
> We could lose someting but gain more thing(TLB flush and unnecessary 
> function call to demote anon pages). 
> 
> I used total_swap_pages because we want to age anon pages 
> even though swap full happens.

We don't have any quantitative data on the effect of these excess tlb
flushes, which makes it difficult to decide which kernel versions
should receive this patch.

Help?

--
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] vmscan: prevent background aging of anon page in no swap system

[Rik van Riel]

On 09/03/2010 05:06 PM, Andrew Morton wrote:

> We don't have any quantitative data on the effect of these excess tlb
> flushes, which makes it difficult to decide which kernel versions
> should receive this patch.
>
> Help?

I assume it is a relatively small performance optimization,
as well as a nice code cleanup ... so probably no real hurry
to get it upstream (next time you send Linus mm patches?),
and no real reason to have it backported to a -stable kernel
either.

-- 
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] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

[-- Attachment #1: Type: text/plain, Size: 2932 bytes --]

On Fri, Sep 3, 2010 at 2:06 PM, Andrew Morton <akpm@linux-foundation.org>wrote:

> On Mon, 30 Aug 2010 00:43:48 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
> > Ying Han reported that backing aging of anon pages in no swap system
> > causes unnecessary TLB flush.
> >
> > When I sent a patch(69c8548175), I wanted this patch but Rik pointed out
> > and allowed aging of anon pages to give a chance to promote from inactive
> > to active LRU.
> >
> > It has a two problem.
> >
> > 1) non-swap system
> >
> > Never make sense to age anon pages.
> >
> > 2) swap configured but still doesn't swapon
> >
> > It doesn't make sense to age anon pages until swap-on time.
> > But it's arguable. If we have aged anon pages by swapon, VM have moved
> > anon pages from active to inactive. And in the time swapon by admin,
> > the VM can't reclaim hot pages so we can protect hot pages swapout.
> >
> > But let's think about it. When does swap-on happen? It depends on admin.
> > we can't expect it. Nonetheless, we have done aging of anon pages to
> > protect hot pages swapout. It means we lost run time overhead when
> > below high watermark but gain hot page swap-[in/out] overhead when VM
> > decide swapout. Is it true? Let's think more detail.
> > We don't promote anon pages in case of non-swap system. So even though
> > VM does aging of anon pages, the pages would be in inactive LRU for a
> long
> > time. It means many of pages in there would mark access bit again. So
> access
> > bit hot/code separation would be pointless.
> >
> > This patch prevents unnecessary anon pages demotion in not-swapon and
> > non-configured swap system. Of course, it could make side effect that
> > hot anon pages could swap out when admin does swap on.
> > But I think sooner or later it would be steady state.
> > So it's not a big problem.
> > We could lose someting but gain more thing(TLB flush and unnecessary
> > function call to demote anon pages).
> >
> > I used total_swap_pages because we want to age anon pages
> > even though swap full happens.
>
> We don't have any quantitative data on the effect of these excess tlb
> flushes, which makes it difficult to decide which kernel versions
> should receive this patch.
>
> Help?
>

Andrew:

We observed the degradation on 2.6.34 compared to 2.6.26 kernel. The
workload we are running is doing 4k-random-write which runs about 3-4
minutes. We captured the TLB shootsdowns before/after:

Before the change:
TLB: 29435 22208 37146 25332 47952 43698 43545 40297 49043 44843 46127 50959
47592 46233 43698 44690 TLB shootdowns [HSUM =  662798 ]

After the change:
TLB: 2340 3113 1547 1472 2944 4194 2181 1212 2607 4373 1690 1446 2310 3784
1744 1134 TLB shootdowns [HSUM =  38091 ]

Also worthy to mention, we are running in fake numa system where each fake
node is 128M size. That makes differences on the check inactive_anon_is_low()
since the active/inactive ratio falls to 1.

--Ying

[-- Attachment #2: Type: text/html, Size: 5386 bytes --]

Re: [PATCH] vmscan: prevent background aging of anon page in no swap system

[Ying Han]

On Fri, Sep 3, 2010 at 2:06 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 30 Aug 2010 00:43:48 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
> > Ying Han reported that backing aging of anon pages in no swap system
> > causes unnecessary TLB flush.
> >
> > When I sent a patch(69c8548175), I wanted this patch but Rik pointed out
> > and allowed aging of anon pages to give a chance to promote from inactive
> > to active LRU.
> >
> > It has a two problem.
> >
> > 1) non-swap system
> >
> > Never make sense to age anon pages.
> >
> > 2) swap configured but still doesn't swapon
> >
> > It doesn't make sense to age anon pages until swap-on time.
> > But it's arguable. If we have aged anon pages by swapon, VM have moved
> > anon pages from active to inactive. And in the time swapon by admin,
> > the VM can't reclaim hot pages so we can protect hot pages swapout.
> >
> > But let's think about it. When does swap-on happen? It depends on admin.
> > we can't expect it. Nonetheless, we have done aging of anon pages to
> > protect hot pages swapout. It means we lost run time overhead when
> > below high watermark but gain hot page swap-[in/out] overhead when VM
> > decide swapout. Is it true? Let's think more detail.
> > We don't promote anon pages in case of non-swap system. So even though
> > VM does aging of anon pages, the pages would be in inactive LRU for a long
> > time. It means many of pages in there would mark access bit again. So access
> > bit hot/code separation would be pointless.
> >
> > This patch prevents unnecessary anon pages demotion in not-swapon and
> > non-configured swap system. Of course, it could make side effect that
> > hot anon pages could swap out when admin does swap on.
> > But I think sooner or later it would be steady state.
> > So it's not a big problem.
> > We could lose someting but gain more thing(TLB flush and unnecessary
> > function call to demote anon pages).
> >
> > I used total_swap_pages because we want to age anon pages
> > even though swap full happens.
>
> We don't have any quantitative data on the effect of these excess tlb
> flushes, which makes it difficult to decide which kernel versions
> should receive this patch.
>
> Help?

Andrew:

We observed the degradation on 2.6.34 compared to 2.6.26 kernel. The
workload we are running is doing 4k-random-write which runs about 3-4
minutes. We captured the TLB shootsdowns before/after:

Before the change:
TLB: 29435 22208 37146 25332 47952 43698 43545 40297 49043 44843 46127
50959 47592 46233 43698 44690 TLB shootdowns [HSUM =  662798 ]

After the change:
TLB: 2340 3113 1547 1472 2944 4194 2181 1212 2607 4373 1690 1446 2310
3784 1744 1134 TLB shootdowns [HSUM =  38091 ]

Also worthy to mention, we are running in fake numa system where each
fake node is 128M size. That makes differences on the check
inactive_anon_is_low() since the active/inactive ratio falls to 1.

--Ying

--
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] vmscan: prevent background aging of anon page in no swap system

[Andrew Morton]

On Fri, 3 Sep 2010 14:47:03 -0700
Ying Han <yinghan@google.com> wrote:

> > We don't have any quantitative data on the effect of these excess tlb
> > flushes, which makes it difficult to decide which kernel versions
> > should receive this patch.
> >
> > Help?
> 
> Andrew:
> 
> We observed the degradation on 2.6.34 compared to 2.6.26 kernel. The
> workload we are running is doing 4k-random-write which runs about 3-4
> minutes. We captured the TLB shootsdowns before/after:
> 
> Before the change:
> TLB: 29435 22208 37146 25332 47952 43698 43545 40297 49043 44843 46127
> 50959 47592 46233 43698 44690 TLB shootdowns [HSUM =  662798 ]
> 
> After the change:
> TLB: 2340 3113 1547 1472 2944 4194 2181 1212 2607 4373 1690 1446 2310
> 3784 1744 1134 TLB shootdowns [HSUM =  38091 ]

Do you have data on how much additional CPU time (and/or wall time) was
consumed?

> Also worthy to mention, we are running in fake numa system where each
> fake node is 128M size. That makes differences on the check
> inactive_anon_is_low() since the active/inactive ratio falls to 1.

--
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] vmscan: prevent background aging of anon page in no swap system

[Venkatesh Pallipadi]

On Fri, Sep 3, 2010 at 2:56 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 3 Sep 2010 14:47:03 -0700
> Ying Han <yinghan@google.com> wrote:
>
>> > We don't have any quantitative data on the effect of these excess tlb
>> > flushes, which makes it difficult to decide which kernel versions
>> > should receive this patch.
>> >
>> > Help?
>>
>> Andrew:
>>
>> We observed the degradation on 2.6.34 compared to 2.6.26 kernel. The
>> workload we are running is doing 4k-random-write which runs about 3-4
>> minutes. We captured the TLB shootsdowns before/after:
>>
>> Before the change:
>> TLB: 29435 22208 37146 25332 47952 43698 43545 40297 49043 44843 46127
>> 50959 47592 46233 43698 44690 TLB shootdowns [HSUM =  662798 ]
>>
>> After the change:
>> TLB: 2340 3113 1547 1472 2944 4194 2181 1212 2607 4373 1690 1446 2310
>> 3784 1744 1134 TLB shootdowns [HSUM =  38091 ]
>
> Do you have data on how much additional CPU time (and/or wall time) was
> consumed?
>

Just reran the workload to get this data
- after - before of /proc/interrupts:TLB
- after - before of /proc/stat:cpu
  (output is: "cpu" user nice sys idle iowait irq softirq steal guest guestnice)

Without this change
TLB: 28550 21232 33876 14300 40661 43118 38227 34887 34376 38208 35735
33591 36305 43649 36558 42013 TLB shootdowns [HSUM =  555286 ]
cpu 41056 381 17945 308706 26447 39 9713 0 0 0

With this change
TLB: 660 1088 761 474 778 1050 697 551 712 1353 651 730 788 1419 574
521 TLB shootdowns [HSUM =  12807 ]
cpu 40375 231 16622 204115 19317 36 9464 0 0 0

This is on a 16 way system, so 16 * 100 count in cpu line above counts as 1s.

I don't think all the reduction in CPU time (especially idle time!)
can be attributed to this change. There is some run to run variation
especially with the setup and teardown of the tests. But, there is a
notable reduction in user, system and irq time. For what its worth,
for this particular workload, throughput number reported by the run is
4% up.

Thanks,
Venki

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