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

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

[Minchan Kim]

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_active_list is called several places.
But if we don't have a swap space, we can't reclaim anon pages.
So, we don't need deactivating anon pages in anon lru list.

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

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

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2f9d555..d7e8242 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1339,8 +1339,7 @@ static int inactive_anon_is_low_global(struct zone *zone)
  * @zone: zone to check
  * @sc:   scan control of this context
  *
- * Returns true if the zone does not have enough inactive anon pages,
- * meaning some active anon pages need to be deactivated.
+ * Returns true if the zone does not have enough inactive anon pages.
  */
 static int inactive_anon_is_low(struct zone *zone, struct scan_control *sc)
 {
@@ -1389,6 +1388,20 @@ static int inactive_file_is_low(struct zone *zone, struct scan_control *sc)
 	return low;
 }
 
+/*
+ * can_reclaim_anon - check if anonymous pages need to be deactivated
+ * @zone: zone to check
+ * @sc:   scan control of this context
+ * 
+ * Returns true if the zone does not have enough inactive anon pages
+ * and have enough swap sppce, meaning some active anon pages need to
+ * be deactivated.
+ */
+static int can_reclaim_anon(struct zone *zone, struct scan_control *sc)
+{
+	return (inactive_anon_is_low(zone, sc) && nr_swap_pages <= 0);
+}
+
 static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
 	struct zone *zone, struct scan_control *sc, int priority)
 {
@@ -1399,7 +1412,7 @@ static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
 		return 0;
 	}
 
-	if (lru == LRU_ACTIVE_ANON && inactive_anon_is_low(zone, sc)) {
+	if (lru == LRU_ACTIVE_ANON && can_reclaim_anon(zone, sc)) {
 		shrink_active_list(nr_to_scan, zone, sc, priority, file);
 		return 0;
 	}
@@ -1577,7 +1590,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 (can_reclaim_anon(zone, sc))
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
 
 	throttle_vm_writeout(sc->gfp_mask);
@@ -1880,7 +1893,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 (can_reclaim_anon(zone, &sc))
 				shrink_active_list(SWAP_CLUSTER_MAX, zone,
 							&sc, priority, 0);
 
-- 
1.5.4.3


-- 
Kinds Regards
Minchan Kim

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

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

[KOSAKI Motohiro]

> 
> 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_active_list is called several places.
> But if we don't have a swap space, we can't reclaim anon pages.
> So, we don't need deactivating anon pages in anon lru list.
> 
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Rik van Riel <riel@redhat.com>	

looks good to me. thanks :)



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

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

[KOSAKI Motohiro]

> > 
> > 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_active_list is called several places.
> > But if we don't have a swap space, we can't reclaim anon pages.
> > So, we don't need deactivating anon pages in anon lru list.
> > 
> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Rik van Riel <riel@redhat.com>	
> 
> looks good to me. thanks :)

Grr, my fault.



>  static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>  	struct zone *zone, struct scan_control *sc, int priority)
>  {
> @@ -1399,7 +1412,7 @@ static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>  		return 0;
>  	}
>  
> -	if (lru == LRU_ACTIVE_ANON && inactive_anon_is_low(zone, sc)) {
> +	if (lru == LRU_ACTIVE_ANON && can_reclaim_anon(zone, sc)) {
>  		shrink_active_list(nr_to_scan, zone, sc, priority, file);
>  		return 0;

you shouldn't do that. if nr_swap_pages==0, get_scan_ratio return anon=0%.
then, this branch is unnecessary.




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

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

[Minchan Kim]

On Thu, May 14, 2009 at 8:44 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> >
>> > 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_active_list is called several places.
>> > But if we don't have a swap space, we can't reclaim anon pages.
>> > So, we don't need deactivating anon pages in anon lru list.
>> >
>> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> > Cc: Johannes Weiner <hannes@cmpxchg.org>
>> > Cc: Rik van Riel <riel@redhat.com>
>>
>> looks good to me. thanks :)
>
> Grr, my fault.
>
>
>
>>  static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>>       struct zone *zone, struct scan_control *sc, int priority)
>>  {
>> @@ -1399,7 +1412,7 @@ static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>>               return 0;
>>       }
>>
>> -     if (lru == LRU_ACTIVE_ANON && inactive_anon_is_low(zone, sc)) {
>> +     if (lru == LRU_ACTIVE_ANON && can_reclaim_anon(zone, sc)) {
>>               shrink_active_list(nr_to_scan, zone, sc, priority, file);
>>               return 0;
>
> you shouldn't do that. if nr_swap_pages==0, get_scan_ratio return anon=0%.
> then, this branch is unnecessary.
>

But, I think at last it can be happen following as.

1515         * Even if we did not try to evict anon pages at all, we want to
1516         * rebalance the anon lru active/inactive ratio.
1517         */
1518        if (inactive_anon_is_low(zone, sc))
1519                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);


>
>



-- 
Kinds regards,
Minchan Kim

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

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

[KOSAKI Motohiro]

> On Thu, May 14, 2009 at 8:44 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> >
> >> > 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_active_list is called several places.
> >> > But if we don't have a swap space, we can't reclaim anon pages.
> >> > So, we don't need deactivating anon pages in anon lru list.
> >> >
> >> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> >> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> >> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> >> > Cc: Rik van Riel <riel@redhat.com>
> >>
> >> looks good to me. thanks :)
> >
> > Grr, my fault.
> >
> >
> >
> >> ?static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
> >> ? ? ? struct zone *zone, struct scan_control *sc, int priority)
> >> ?{
> >> @@ -1399,7 +1412,7 @@ static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
> >> ? ? ? ? ? ? ? return 0;
> >> ? ? ? }
> >>
> >> - ? ? if (lru == LRU_ACTIVE_ANON && inactive_anon_is_low(zone, sc)) {
> >> + ? ? if (lru == LRU_ACTIVE_ANON && can_reclaim_anon(zone, sc)) {
> >> ? ? ? ? ? ? ? shrink_active_list(nr_to_scan, zone, sc, priority, file);
> >> ? ? ? ? ? ? ? return 0;
> >
> > you shouldn't do that. if nr_swap_pages==0, get_scan_ratio return anon=0%.
> > then, this branch is unnecessary.
> >
> 
> But, I think at last it can be happen following as.
> 
> 1515         * Even if we did not try to evict anon pages at all, we want to
> 1516         * rebalance the anon lru active/inactive ratio.
> 1517         */
> 1518        if (inactive_anon_is_low(zone, sc))
> 1519                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

I pointed to shrink_list(), but you replayed shrink_zone().
I only talked about shrink_list().


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

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

[Minchan Kim]

On Thu, May 14, 2009 at 9:11 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Thu, May 14, 2009 at 8:44 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> >> >
>> >> > 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_active_list is called several places.
>> >> > But if we don't have a swap space, we can't reclaim anon pages.
>> >> > So, we don't need deactivating anon pages in anon lru list.
>> >> >
>> >> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>> >> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> >> > Cc: Johannes Weiner <hannes@cmpxchg.org>
>> >> > Cc: Rik van Riel <riel@redhat.com>
>> >>
>> >> looks good to me. thanks :)
>> >
>> > Grr, my fault.
>> >
>> >
>> >
>> >> ?static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>> >> ? ? ? struct zone *zone, struct scan_control *sc, int priority)
>> >> ?{
>> >> @@ -1399,7 +1412,7 @@ static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>> >> ? ? ? ? ? ? ? return 0;
>> >> ? ? ? }
>> >>
>> >> - ? ? if (lru == LRU_ACTIVE_ANON && inactive_anon_is_low(zone, sc)) {
>> >> + ? ? if (lru == LRU_ACTIVE_ANON && can_reclaim_anon(zone, sc)) {
>> >> ? ? ? ? ? ? ? shrink_active_list(nr_to_scan, zone, sc, priority, file);
>> >> ? ? ? ? ? ? ? return 0;
>> >
>> > you shouldn't do that. if nr_swap_pages==0, get_scan_ratio return anon=0%.
>> > then, this branch is unnecessary.
>> >
>>
>> But, I think at last it can be happen following as.
>>
>> 1515         * Even if we did not try to evict anon pages at all, we want to
>> 1516         * rebalance the anon lru active/inactive ratio.
>> 1517         */
>> 1518        if (inactive_anon_is_low(zone, sc))
>> 1519                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
>
> I pointed to shrink_list(), but you replayed shrink_zone().
> I only talked about shrink_list().
>

In shrink_zone, we call get_scan_ratio. it prevent scanning anon list.
but, in shrink_all_zones can't prevent it.

Also,  I think shrink_list is not hot patch.
So check of one condition adding is trivial

If I don't understand your point, please explain detaily


-- 
Kinds regards,
Minchan Kim

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

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

[Rik van Riel]

Minchan Kim wrote:

> Now shrink_active_list is called several places.
> But if we don't have a swap space, we can't reclaim anon pages.

If swap space has run out, get_scan_ratio() will return
0 for the anon scan ratio, meaning we do not scan the
anon lists.

> So, we don't need deactivating anon pages in anon lru list.

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.

> +static int can_reclaim_anon(struct zone *zone, struct scan_control *sc)
> +{
> +	return (inactive_anon_is_low(zone, sc) && nr_swap_pages <= 0);
> +}
> +

This function name is misleading, because when we do have
swap space available but inactive_anon_is_low is false,
we still want to reclaim inactive anon pages!

What problem did you encounter that you think this patch
solves?

-- 
All rights reversed.

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

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

[Minchan Kim]

HI, Rik

Thanks for careful review. :)

On Thu, May 14, 2009 at 9:58 PM, Rik van Riel <riel@redhat.com> wrote:
> Minchan Kim wrote:
>
>> Now shrink_active_list is called several places.
>> But if we don't have a swap space, we can't reclaim anon pages.
>
> If swap space has run out, get_scan_ratio() will return
> 0 for the anon scan ratio, meaning we do not scan the
> anon lists.

I think get_scan_ration can't prevent scanning of anon pages in no
swap system(like embedded system).
That's because in shrink_zone, you add following as

        /*
         * 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))
                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

>> So, we don't need deactivating anon pages in anon lru list.
>
> 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.

I agree your opinion.

>> +static int can_reclaim_anon(struct zone *zone, struct scan_control *sc)
>> +{
>> +       return (inactive_anon_is_low(zone, sc) && nr_swap_pages <= 0);
>> +}
>> +
>
> This function name is misleading, because when we do have
> swap space available but inactive_anon_is_low is false,
> we still want to reclaim inactive anon pages!

Indeed. I will rename it.

> What problem did you encounter that you think this patch
> solves?

I thought In embedded system most products don't have swap space.
In such environment, We don't need anon lru list.
I think even scanning of anon list is much bad


> --
> All rights reversed.
>



-- 
Kinds regards,
Minchan Kim

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

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

[Rik van Riel]

Minchan Kim wrote:
> HI, Rik
> 
> Thanks for careful review. :)
> 
> On Thu, May 14, 2009 at 9:58 PM, Rik van Riel <riel@redhat.com> wrote:
>> Minchan Kim wrote:
>>
>>> Now shrink_active_list is called several places.
>>> But if we don't have a swap space, we can't reclaim anon pages.
>> If swap space has run out, get_scan_ratio() will return
>> 0 for the anon scan ratio, meaning we do not scan the
>> anon lists.
> 
> I think get_scan_ration can't prevent scanning of anon pages in no
> swap system(like embedded system).
> That's because in shrink_zone, you add following as
> 
>         /*
>          * 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))
>                 shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);

That's a fair point.

How about we change this to:

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

That way GCC will statically optimize away this branch on
systems with CONFIG_SWAP=n.

Does that look reasonable?

-- 
All rights reversed.

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

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

[Minchan Kim]

On Thu, May 14, 2009 at 10:18 PM, Rik van Riel <riel@redhat.com> wrote:
> Minchan Kim wrote:
>>
>> HI, Rik
>>
>> Thanks for careful review. :)
>>
>> On Thu, May 14, 2009 at 9:58 PM, Rik van Riel <riel@redhat.com> wrote:
>>>
>>> Minchan Kim wrote:
>>>
>>>> Now shrink_active_list is called several places.
>>>> But if we don't have a swap space, we can't reclaim anon pages.
>>>
>>> If swap space has run out, get_scan_ratio() will return
>>> 0 for the anon scan ratio, meaning we do not scan the
>>> anon lists.
>>
>> I think get_scan_ration can't prevent scanning of anon pages in no
>> swap system(like embedded system).
>> That's because in shrink_zone, you add following as
>>
>>        /*
>>         * 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))
>>                shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority,
>> 0);
>
> That's a fair point.
>
> How about we change this to:
>
>        if (inactive_anon_is_low(zone, sc) && nr_swap_pages >= 0)
> That way GCC will statically optimize away this branch on
> systems with CONFIG_SWAP=n.
>
> Does that look reasonable?

Now inactive_anon_is_low called following as.

1. shrink_zone => Looks good since your idea.
2. balance_pgdat => Looks good since aging.
3. shrink_list
shrink_list is called at two places.
1. shrink_zone => It's OK since get_scan_ratio can't prevent it.
2. shrink_all_zones. => It's OK since we can't suspend without swap space.

So, Okay I will do that in next version.
Thanks for good review. Rik :)
Could I add your ack in next version ?

> --
> All rights reversed.
>



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