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 <[email protected]>
Cc: KOSAKI Motohiro <[email protected]>
Cc: Johannes Weiner <[email protected]>
Reported-by: Ying Han <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
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
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<[email protected]>
> Cc: KOSAKI Motohiro<[email protected]>
> Cc: Johannes Weiner<[email protected]>
> Reported-by: Ying Han<[email protected]>
> Signed-off-by: Minchan Kim<[email protected]>
Reviewed-by: Rik van Riel <[email protected]>
--
All rights reversed
On Sun, Aug 29, 2010 at 8:43 AM, Minchan Kim <[email protected]> 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 <[email protected]>
> Cc: KOSAKI Motohiro <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Reported-by: Ying Han <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> ?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
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
On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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
>
On 08/29/2010 05:23 PM, Ying Han wrote:
> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel<[email protected]> 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
Hi Ying,
On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <[email protected]> wrote:
> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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
On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <[email protected]> wrote:
> Hi Ying,
>
> On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <[email protected]> wrote:
>> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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
>
On Mon, Aug 30, 2010 at 2:40 PM, Ying Han <[email protected]> wrote:
> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <[email protected]> wrote:
>> Hi Ying,
>>
>> On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <[email protected]> wrote:
>>> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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
> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <[email protected]> wrote:
> > Hi Ying,
> >
> > On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <[email protected]> wrote:
> >> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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 <[email protected]>
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.
> 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
Hi, KOSAKI.
On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
<[email protected]> 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
> Hi, KOSAKI.
>
> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
> <[email protected]> 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?
On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Sun, Aug 29, 2010 at 5:18 PM, Minchan Kim <[email protected]> wrote:
>> > Hi Ying,
>> >
>> > On Mon, Aug 30, 2010 at 6:23 AM, Ying Han <[email protected]> wrote:
>> >> On Sun, Aug 29, 2010 at 1:03 PM, Rik van Riel <[email protected]> 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 <[email protected]>
>
>
> 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
On Tue, Aug 31, 2010 at 10:18 AM, KOSAKI Motohiro
<[email protected]> wrote:
>> Hi, KOSAKI.
>>
>> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
>> <[email protected]> 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
> > 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 :)
> On Tue, Aug 31, 2010 at 10:18 AM, KOSAKI Motohiro
> <[email protected]> wrote:
> >> Hi, KOSAKI.
> >>
> >> On Tue, Aug 31, 2010 at 9:56 AM, KOSAKI Motohiro
> >> <[email protected]> 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.
On Tue, Aug 31, 2010 at 10:38 AM, KOSAKI Motohiro
<[email protected]> 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 <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
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
> How about this?
>
> (Not formal patch. If we agree, I will post it later when I have a SMTP).
>
>
> Signed-off-by: Ying Han <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
>
> 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 :)
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
On Tue, Aug 31, 2010 at 11:30 AM, Rik van Riel <[email protected]> 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
On Tue, Aug 31, 2010 at 11:09 AM, KOSAKI Motohiro
<[email protected]> 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 <[email protected]>
>> Signed-off-by: Minchan Kim <[email protected]>
>>
>> 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