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

PATCH In review MinChan Kim 7 participants 11 messages May 14, 2009 – May 18, 2009

Subsystems (3)

Memory management MGLRU MM reclaim

lore

Share & tools

lore

MinChan Kim

author

2009-05-14 14:15 UTC #0 lore raw

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

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

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

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

Now shrink_zone can deactivate active anon pages even if we don't have a swap device. Many embedded products don't have a swap device. So the deactivation of anon pages is unnecessary.

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

Thanks for good review. Rik and Kosaki.

Signed-off-by Minchan Kim <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

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

mm/vmscan.c +1 -1

index 2f9d555..621708f 100644
@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
 	 * Even if we did not try to evict anon pages at all, we want to
 	 * rebalance the anon lru active/inactive ratio.
 	 */
-	if (inactive_anon_is_low(zone, sc))
+	if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
 
 	throttle_vm_writeout(sc->gfp_mask);

Signature · 12 lines

1.5.4.3


-- 
Kinds Regards,
Minchan Kim 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

2009-05-14 14:39 UTC #2 lore raw

Quoted 18 lines · KOSAKI Motohiro

Quoted 8 lines

mm/vmscan.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/vmscan.c +0 -0
index 2f9d555..621708f 100644

mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1581

Quoted 6 lines

mm/vmscan.c +1 -1

@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
         * Even if we did not try to evict anon pages at all, we want to
         * rebalance the anon lru active/inactive ratio.
         */
-       if (inactive_anon_is_low(zone, sc))
+       if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

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

Quoted 2 lines

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

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

Signature · 2 lines

Thanks,
Minchan Kim

KOSAKI Motohiro

2009-05-14 14:54 UTC #3 lore raw

2009/5/14 Minchan Kim <[email protected]>:

Quoted 20 lines

Quoted 18 lines · KOSAKI Motohiro

Quoted 8 lines

mm/vmscan.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/vmscan.c +0 -0
index 2f9d555..621708f 100644

mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1581

Quoted 6 lines

mm/vmscan.c +1 -1

@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
         * Even if we did not try to evict anon pages at all, we want to
         * rebalance the anon lru active/inactive ratio.
         */
-       if (inactive_anon_is_low(zone, sc))
+       if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

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

Quoted 5 lines

Quoted 2 lines
is better?
compiler can't swap evaluate order around &&.
If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned, we don't have a concern.

ok. I ack this.

Johannes Weiner

maintainer

2009-05-14 16:24 UTC #4 lore raw

In reply to Minchan Kim

Quoted 20 lines · Minchan Kim

Quoted 18 lines · KOSAKI Motohiro
Quoted 13 lines
A mm/vmscan.c | A A 2 +-
A 1 files changed, 1 insertions(+), 1 deletions(-)
mm/vmscan.c +0 -0
index 2f9d555..621708f 100644
@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
A  A  A  A  * Even if we did not try to evict anon pages at all, we want to
A  A  A  A  * rebalance the anon lru active/inactive ratio.
A  A  A  A  */
- A  A  A  if (inactive_anon_is_low(zone, sc))
+ A  A  A  if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
Quoted 1 line
A A A A A A A A shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
A A A if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))

Quoted 5 lines

Quoted 2 lines
is better?
compiler can't swap evaluate order around &&.
If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned, we don't have a concern.

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

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

	atomic_read() && 0

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

Compared to

	0 && atomic_read()

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

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

Hannes

Minchan Kim

author

2009-05-15 01:28 UTC #5 lore raw

Quoted 41 lines · Johannes Weiner

Quoted 20 lines · Minchan Kim
Quoted 18 lines · KOSAKI Motohiro
Quoted 8 lines
mm/vmscan.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/vmscan.c +0 -0
index 2f9d555..621708f 100644
mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1581
Quoted 6 lines
mm/vmscan.c +1 -1
@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
         * Even if we did not try to evict anon pages at all, we want to
         * rebalance the anon lru active/inactive ratio.
         */
-       if (inactive_anon_is_low(zone, sc))
+       if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
if (nr_swap_pages > 0 && inactive_anon_is_low(zone, sc))
Quoted 5 lines
Quoted 2 lines
is better?
compiler can't swap evaluate order around &&.
If GCC optimizes away that branch with CONFIG_SWAP=n as Rik mentioned, we don't have a concern.
It can only optimize it away when the condition is a compile time constant.
But inactive_anon_is_low() contains atomic operations which the compiler is not allowed to drop and so the && semantics lead to
atomic_read() && 0
emitting the read while still knowing the whole expression is 0 at compile-time, optimizing away only the branch itself but leaving the read in place!
Compared to

Quoted 7 lines

0 && atomic_read()
where the && short-circuitry leads to atomic_read() not being executed. And since the 0 is a compile time constant, no code has to be emitted for the read.
So KOSAKI-san's is right. Your version results in bigger object code.

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

Quoted 2 lines

Hannes

Signature · 8 lines

Kinds regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rik van Riel

Acked-by on v1 1/1

2009-05-14 15:35 UTC #6 lore raw

In reply to MinChan Kim

Quoted 6 lines · MinChan Kim

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

mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1583

Quoted 8 lines

mm/vmscan.c +1 -1

@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
 	 * Even if we did not try to evict anon pages at all, we want to
 	 * rebalance the anon lru active/inactive ratio.
 	 */
-	if (inactive_anon_is_low(zone, sc))
+	if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
 
 	throttle_vm_writeout(sc->gfp_mask);

Acked-by Rik van Riel <[email protected]>

Andrew Morton

maintainer

2009-05-14 16:31 UTC #7 lore raw

In reply to MinChan Kim

Quoted 2 lines · On Thu, 14 May 2009 23:15:55 +0900 MinChan Kim <[email protected]> wrote:

Now shrink_zone can deactivate active anon pages even if we don't have a swap device. Many embedded products don't have a swap device. So the deactivation of anon pages is unnecessary.

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

Rik van Riel

2009-05-14 17:25 UTC #8 lore raw

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Thu, 14 May 2009 23:15:55 +0900 MinChan Kim <[email protected]> wrote:
Now shrink_zone can deactivate active anon pages even if we don't have a swap device. Many embedded products don't have a swap device. So the deactivation of anon pages is unnecessary.
Does shrink_list() need to scan the anon LRUs at all if there's no swap online?

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

Wu Fengguang

Acked-by on v1 1/1

2009-05-18 02:33 UTC #9 lore raw

In reply to MinChan Kim

mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1581

Quoted 11 lines · MinChan Kim

mm/vmscan.c +1 -1

index 2f9d555..621708f 100644
@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
 	 * Even if we did not try to evict anon pages at all, we want to
 	 * rebalance the anon lru active/inactive ratio.
 	 */
-	if (inactive_anon_is_low(zone, sc))
+	if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

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

Acked-by Wu Fengguang <[email protected]>

Minchan Kim

author

2009-05-18 02:40 UTC #10 lore raw

Quoted 16 lines · On Mon, 18 May 2009 10:34:04 +0800 Wu Fengguang <[email protected]> wrote:

mm/vmscan.c static void shrink_zone(int priority, struct zone *zone, · line 1581
Quoted 11 lines · MinChan Kim
mm/vmscan.c +1 -1
index 2f9d555..621708f 100644
@@ -1577,7 +1577,7 @@ static void shrink_zone(int priority, struct zone *zone,
 	 * Even if we did not try to evict anon pages at all, we want to
 	 * rebalance the anon lru active/inactive ratio.
 	 */
-	if (inactive_anon_is_low(zone, sc))
+	if (inactive_anon_is_low(zone, sc) && nr_swap_pages > 0)
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
There's another "if (inactive_anon_is_low) shrink_active_list;" occurrence to be fixed in balance_pgdat()? Otherwise:

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

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

Quoted 1 line

Acked-by Wu Fengguang <[email protected]>

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

Signature · 8 lines

Kinds Regards
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>