* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-30 5:40 ` Ying Han
0 siblings, 0 replies; 56+ messages in thread
From: Ying Han @ 2010-08-30 5:40 UTC (permalink / raw)
To: Minchan Kim
Cc: Rik van Riel, Andrew Morton, linux-mm, LKML, Venkatesh Pallipadi,
KOSAKI Motohiro, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-30 5:40 ` Ying Han
@ 2010-08-30 6:16 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-30 6:16 UTC (permalink / raw)
To: Ying Han
Cc: Rik van Riel, Andrew Morton, linux-mm, LKML, Venkatesh Pallipadi,
KOSAKI Motohiro, Johannes Weiner
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-30 6:16 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-30 6:16 UTC (permalink / raw)
To: Ying Han
Cc: Rik van Riel, Andrew Morton, linux-mm, LKML, Venkatesh Pallipadi,
KOSAKI Motohiro, Johannes Weiner
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>
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-30 6:16 ` Minchan Kim
@ 2010-08-31 0:56 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 0:56 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 0:56 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 0:56 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 0:56 ` KOSAKI Motohiro
@ 2010-08-31 1:10 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:10 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:10 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:10 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:10 ` Minchan Kim
@ 2010-08-31 1:18 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:18 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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?
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:18 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:18 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:18 ` KOSAKI Motohiro
@ 2010-08-31 1:36 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:36 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:36 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:36 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:36 ` Minchan Kim
@ 2010-08-31 1:41 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:41 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:41 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:41 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-30 5:40 ` Ying Han
@ 2010-08-31 0:56 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 0:56 UTC (permalink / raw)
To: Ying Han
Cc: kosaki.motohiro, Minchan Kim, Rik van Riel, Andrew Morton,
linux-mm, LKML, Venkatesh Pallipadi, Johannes Weiner
> 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.
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 0:56 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 0:56 UTC (permalink / raw)
To: Ying Han
Cc: kosaki.motohiro, Minchan Kim, Rik van Riel, Andrew Morton,
linux-mm, LKML, Venkatesh Pallipadi, Johannes Weiner
> 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>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 0:56 ` KOSAKI Motohiro
@ 2010-08-31 1:23 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:23 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:23 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 1:23 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:23 ` Minchan Kim
@ 2010-08-31 1:38 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:38 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> > 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 :)
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 1:38 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 1:38 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> > 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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:38 ` KOSAKI Motohiro
@ 2010-08-31 2:02 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 2:02 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 2:02 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 2:02 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 2:02 ` Minchan Kim
@ 2010-08-31 2:09 ` KOSAKI Motohiro
-1 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 2:09 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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 :)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 2:09 ` KOSAKI Motohiro
0 siblings, 0 replies; 56+ messages in thread
From: KOSAKI Motohiro @ 2010-08-31 2:09 UTC (permalink / raw)
To: Minchan Kim
Cc: kosaki.motohiro, Ying Han, Rik van Riel, Andrew Morton, linux-mm,
LKML, Venkatesh Pallipadi, Johannes Weiner
> 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>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 2:09 ` KOSAKI Motohiro
@ 2010-08-31 3:47 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 3:47 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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).
>>
>>
>> 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 ?
Yes. :)
>
>
>
>> +
>> 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 :)
>
Thanks.
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 3:47 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 3:47 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Ying Han, Rik van Riel, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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)
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 1:23 ` Minchan Kim
@ 2010-08-31 2:30 ` Rik van Riel
-1 siblings, 0 replies; 56+ messages in thread
From: Rik van Riel @ 2010-08-31 2:30 UTC (permalink / raw)
To: Minchan Kim
Cc: KOSAKI Motohiro, Ying Han, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 2:30 ` Rik van Riel
0 siblings, 0 replies; 56+ messages in thread
From: Rik van Riel @ 2010-08-31 2:30 UTC (permalink / raw)
To: Minchan Kim
Cc: KOSAKI Motohiro, Ying Han, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
2010-08-31 2:30 ` Rik van Riel
@ 2010-08-31 3:46 ` Minchan Kim
-1 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 3:46 UTC (permalink / raw)
To: Rik van Riel
Cc: KOSAKI Motohiro, Ying Han, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH] vmscan: prevent background aging of anon page in no swap system
@ 2010-08-31 3:46 ` Minchan Kim
0 siblings, 0 replies; 56+ messages in thread
From: Minchan Kim @ 2010-08-31 3:46 UTC (permalink / raw)
To: Rik van Riel
Cc: KOSAKI Motohiro, Ying Han, Andrew Morton, linux-mm, LKML,
Venkatesh Pallipadi, Johannes Weiner
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>
^ permalink raw reply [flat|nested] 56+ messages in thread