[PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Ying Han]

This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.

Before the commit, the code makes senses to me but not after the commit. The
"nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
lru lists. The "nr_to_reclaim" is the target value for the whole function. For
example, we like to early break the reclaim if reclaimed 32 pages under direct
reclaim (not DEF_PRIORITY).

After the reverted commit, the target "nr_to_reclaim" is decremented each time
by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
doesn't make sense to me...

Signed-off-by: Ying Han <yinghan@google.com>
---
 mm/vmscan.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 33c332b..1a51868 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2107,12 +2107,7 @@ restart:
 		 * with multiple processes reclaiming pages, the total
 		 * freeing target can get unreasonably large.
 		 */
-		if (nr_reclaimed >= nr_to_reclaim)
-			nr_to_reclaim = 0;
-		else
-			nr_to_reclaim -= nr_reclaimed;
-
-		if (!nr_to_reclaim && priority < DEF_PRIORITY)
+		if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
 			break;
 	}
 	blk_finish_plug(&plug);
-- 
1.7.7.3

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Andrew Morton]

On Mon,  9 Apr 2012 12:42:04 -0700
Ying Han <yinghan@google.com> wrote:

> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
> 
> Before the commit, the code makes senses to me but not after the commit. The
> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
> example, we like to early break the reclaim if reclaimed 32 pages under direct
> reclaim (not DEF_PRIORITY).
> 
> After the reverted commit, the target "nr_to_reclaim" is decremented each time
> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
> doesn't make sense to me...
> 
> Signed-off-by: Ying Han <yinghan@google.com>
> ---
>  mm/vmscan.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 33c332b..1a51868 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2107,12 +2107,7 @@ restart:
>  		 * with multiple processes reclaiming pages, the total
>  		 * freeing target can get unreasonably large.
>  		 */
> -		if (nr_reclaimed >= nr_to_reclaim)
> -			nr_to_reclaim = 0;
> -		else
> -			nr_to_reclaim -= nr_reclaimed;
> -
> -		if (!nr_to_reclaim && priority < DEF_PRIORITY)
> +		if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>  			break;
>  	}
>  	blk_finish_plug(&plug);

This code is all within a loop: the "goto restart" thing.  We reset
nr_reclaimed to zero each time around that loop.  nr_to_reclaim is (or
rather, was) constant throughout the entire function.

Comparing nr_reclaimed (whcih is reset each time around the loop) to
nr_to_reclaim made no sense.

I think the code as it stands is ugly.  It would be better to make
nr_to_reclaim a const and to add another local total_reclaimed, and
compare that with nr_to_reclaim.  Or just stop resetting nr_reclaimed
each time around the loop.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Ying Han]

On Mon, Apr 9, 2012 at 12:50 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon,  9 Apr 2012 12:42:04 -0700
> Ying Han <yinghan@google.com> wrote:
>
>> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
>>
>> Before the commit, the code makes senses to me but not after the commit. The
>> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
>> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
>> example, we like to early break the reclaim if reclaimed 32 pages under direct
>> reclaim (not DEF_PRIORITY).
>>
>> After the reverted commit, the target "nr_to_reclaim" is decremented each time
>> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
>> doesn't make sense to me...
>>
>> Signed-off-by: Ying Han <yinghan@google.com>
>> ---
>>  mm/vmscan.c |    7 +------
>>  1 files changed, 1 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 33c332b..1a51868 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2107,12 +2107,7 @@ restart:
>>                * with multiple processes reclaiming pages, the total
>>                * freeing target can get unreasonably large.
>>                */
>> -             if (nr_reclaimed >= nr_to_reclaim)
>> -                     nr_to_reclaim = 0;
>> -             else
>> -                     nr_to_reclaim -= nr_reclaimed;
>> -
>> -             if (!nr_to_reclaim && priority < DEF_PRIORITY)
>> +             if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>>                       break;
>>       }
>>       blk_finish_plug(&plug);
>
> This code is all within a loop: the "goto restart" thing.  We reset
> nr_reclaimed to zero each time around that loop.  nr_to_reclaim is (or
> rather, was) constant throughout the entire function.
>
> Comparing nr_reclaimed (whcih is reset each time around the loop) to
> nr_to_reclaim made no sense.
>
> I think the code as it stands is ugly.  It would be better to make
> nr_to_reclaim a const and to add another local total_reclaimed, and
> compare that with nr_to_reclaim.

Ok, I will resend the patch w/ the "total_reclaimed" change.

--Ying

Or just stop resetting nr_reclaimed
> each time around the loop.
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Ying Han]

On Mon, Apr 9, 2012 at 2:23 PM, Ying Han <yinghan@google.com> wrote:
> On Mon, Apr 9, 2012 at 12:50 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon,  9 Apr 2012 12:42:04 -0700
>> Ying Han <yinghan@google.com> wrote:
>>
>>> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
>>>
>>> Before the commit, the code makes senses to me but not after the commit. The
>>> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
>>> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
>>> example, we like to early break the reclaim if reclaimed 32 pages under direct
>>> reclaim (not DEF_PRIORITY).
>>>
>>> After the reverted commit, the target "nr_to_reclaim" is decremented each time
>>> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
>>> doesn't make sense to me...
>>>
>>> Signed-off-by: Ying Han <yinghan@google.com>
>>> ---
>>>  mm/vmscan.c |    7 +------
>>>  1 files changed, 1 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 33c332b..1a51868 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -2107,12 +2107,7 @@ restart:
>>>                * with multiple processes reclaiming pages, the total
>>>                * freeing target can get unreasonably large.
>>>                */
>>> -             if (nr_reclaimed >= nr_to_reclaim)
>>> -                     nr_to_reclaim = 0;
>>> -             else
>>> -                     nr_to_reclaim -= nr_reclaimed;
>>> -
>>> -             if (!nr_to_reclaim && priority < DEF_PRIORITY)
>>> +             if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>>>                       break;
>>>       }
>>>       blk_finish_plug(&plug);
>>
>> This code is all within a loop: the "goto restart" thing.  We reset
>> nr_reclaimed to zero each time around that loop.  nr_to_reclaim is (or
>> rather, was) constant throughout the entire function.
>>
>> Comparing nr_reclaimed (whcih is reset each time around the loop) to
>> nr_to_reclaim made no sense.
>>
>> I think the code as it stands is ugly.  It would be better to make
>> nr_to_reclaim a const and to add another local total_reclaimed, and
>> compare that with nr_to_reclaim.
>
> Ok, I will resend the patch w/ the "total_reclaimed" change.
>
> --Ying


I have the patch ready but I am not sure if that is what we want. If
we use total_reclaimed to compare w/ nr_to_reclaim, we end up reducing
the amount of work to reclaim before
compaction(should_continue_reclaim() is true case).

--Ying

>
> Or just stop resetting nr_reclaimed
>> each time around the loop.
>>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Hugh Dickins]

On Mon, 9 Apr 2012, Andrew Morton wrote:
> On Mon,  9 Apr 2012 12:42:04 -0700
> Ying Han <yinghan@google.com> wrote:
> 
> > This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
> > 
> > Before the commit, the code makes senses to me but not after the commit. The
> > "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
> > lru lists. The "nr_to_reclaim" is the target value for the whole function. For
> > example, we like to early break the reclaim if reclaimed 32 pages under direct
> > reclaim (not DEF_PRIORITY).
> > 
> > After the reverted commit, the target "nr_to_reclaim" is decremented each time
> > by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
> > doesn't make sense to me...
> > 
> > Signed-off-by: Ying Han <yinghan@google.com>

Acked-by: Hugh Dickins <hughd@google.com>

though I do prefer the revert to the description of it.

> > ---
> >  mm/vmscan.c |    7 +------
> >  1 files changed, 1 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 33c332b..1a51868 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2107,12 +2107,7 @@ restart:
> >  		 * with multiple processes reclaiming pages, the total
> >  		 * freeing target can get unreasonably large.
> >  		 */
> > -		if (nr_reclaimed >= nr_to_reclaim)
> > -			nr_to_reclaim = 0;
> > -		else
> > -			nr_to_reclaim -= nr_reclaimed;
> > -
> > -		if (!nr_to_reclaim && priority < DEF_PRIORITY)
> > +		if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
> >  			break;
> >  	}
> >  	blk_finish_plug(&plug);
> 
> This code is all within a loop: the "goto restart" thing.  We reset
> nr_reclaimed to zero each time around that loop.  nr_to_reclaim is (or
> rather, was) constant throughout the entire function.
> 
> Comparing nr_reclaimed (whcih is reset each time around the loop) to
> nr_to_reclaim made no sense.

The "restart: nr_reclaimed = 0; ... if should_continue_reclaim goto restart;"
business is a "late" addition for the exceptional case of compaction.
It makes sense to me as "But in the high-order compaction case, we may need
to try N times as hard as the caller asked for: go round and do it again".

If you set aside the restart business, and look at the usual "while (nr..."
loop, c38446 makes little sense.  Each time around that loop, nr_reclaimed
goes up by the amount you'd expect, and nr_to_reclaim goes down by
nr_reclaimed i.e. by a larger and larger amount each time around the
loop (if we assume at least one page is reclaimed each time around).

Now, it's possible that that interesting nonlinearity is precisely
the magic needed for optimal behaviour in shrink_mem_cgroup_zone();
but the commit comment doesn't claim that, it claims to be correcting
a mistake in nr_reclaimed versus nr_to_reclaim (perceived, I guess,
in the exceptional restart loop), whereas it's introducing a mistake
in nr_reclaimed versus nr_to_reclaim in the common while loop.

> 
> I think the code as it stands is ugly.  It would be better to make
> nr_to_reclaim a const and to add another local total_reclaimed, and
> compare that with nr_to_reclaim.  Or just stop resetting nr_reclaimed
> each time around the loop.

I bet you're right that it could be improved, in clarity and in function;
but I'd rather leave that to someone who knows what they're doing: there's
no end to the doubts here (I get hung up on sc->nr_reclaimed, which long
long ago was set to nr_reclaimed here, but nowadays is incremented, and
I wonder whether it gets reset appropriately).  Get into total_reclaimed
and you start down the line of functional change here, without adequate
testing.

For now let's just take the first step of reverting the mistaken commit.

Hugh

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Andrew Morton]

On Mon, 9 Apr 2012 16:24:16 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 33c332b..1a51868 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -2107,12 +2107,7 @@ restart:
> > >  		 * with multiple processes reclaiming pages, the total
> > >  		 * freeing target can get unreasonably large.
> > >  		 */
> > > -		if (nr_reclaimed >= nr_to_reclaim)
> > > -			nr_to_reclaim = 0;
> > > -		else
> > > -			nr_to_reclaim -= nr_reclaimed;
> > > -
> > > -		if (!nr_to_reclaim && priority < DEF_PRIORITY)
> > > +		if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
> > >  			break;
> > >  	}
> > >  	blk_finish_plug(&plug);
> > 
> > This code is all within a loop: the "goto restart" thing.  We reset
> > nr_reclaimed to zero each time around that loop.  nr_to_reclaim is (or
> > rather, was) constant throughout the entire function.
> > 
> > Comparing nr_reclaimed (whcih is reset each time around the loop) to
> > nr_to_reclaim made no sense.
> 
> The "restart: nr_reclaimed = 0; ... if should_continue_reclaim goto restart;"
> business is a "late" addition for the exceptional case of compaction.
> It makes sense to me as "But in the high-order compaction case, we may need
> to try N times as hard as the caller asked for: go round and do it again".
> 
> If you set aside the restart business, and look at the usual "while (nr..."
> loop, c38446 makes little sense.  Each time around that loop, nr_reclaimed
> goes up by the amount you'd expect, and nr_to_reclaim goes down by
> nr_reclaimed i.e. by a larger and larger amount each time around the
> loop (if we assume at least one page is reclaimed each time around).

Oh, yes, true - it's the loop-within-the-loop.

> > I think the code as it stands is ugly.  It would be better to make
> > nr_to_reclaim a const and to add another local total_reclaimed, and
> > compare that with nr_to_reclaim.  Or just stop resetting nr_reclaimed
> > each time around the loop.
> 
> I bet you're right that it could be improved, in clarity and in function;
> but I'd rather leave that to someone who knows what they're doing: there's
> no end to the doubts here (I get hung up on sc->nr_reclaimed, which long
> long ago was set to nr_reclaimed here, but nowadays is incremented, and
> I wonder whether it gets reset appropriately).  Get into total_reclaimed
> and you start down the line of functional change here, without adequate
> testing.
> 

So for compaction, we go around and try to reclaim another
nr_to_reclaim hunk of pages.  The (re)use of nr_to_reclaim seems rather
arbitrary here.  Particularly as nr_reclaimed and nr_to_reclaim don't
actually do anything for low-priority scanning.  I guess it doesn't
matter much, as long as we don't go and scan far too many pages.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Hillf Danton]

On Tue, Apr 10, 2012 at 3:42 AM, Ying Han <yinghan@google.com> wrote:
> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
>
> Before the commit, the code makes senses to me but not after the commit. The
> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
> example, we like to early break the reclaim if reclaimed 32 pages under direct
> reclaim (not DEF_PRIORITY).
>
> After the reverted commit, the target "nr_to_reclaim" is decremented each time
> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
> doesn't make sense to me...
>
I downloaded mm/vmscan.c from the next tree a couple minutes ago, and
see
		.nr_to_reclaim = SWAP_CLUSTER_MAX,
and
	nr_reclaimed = do_try_to_free_pages(zonelist, &sc, &shrink);

in try_to_free_pages().

I also see
		total_scanned += sc->nr_scanned;
		if (sc->nr_reclaimed >= sc->nr_to_reclaim)
			goto out;

in do_try_to_free_pages(),

then would you please say a few words about the sense
of the check of nr_to_reclaim?


> Signed-off-by: Ying Han <yinghan@google.com>
> ---
>  mm/vmscan.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 33c332b..1a51868 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2107,12 +2107,7 @@ restart:
>                 * with multiple processes reclaiming pages, the total
>                 * freeing target can get unreasonably large.
>                 */
> -               if (nr_reclaimed >= nr_to_reclaim)
> -                       nr_to_reclaim = 0;
> -               else
> -                       nr_to_reclaim -= nr_reclaimed;
> -
> -               if (!nr_to_reclaim && priority < DEF_PRIORITY)
> +               if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>                        break;
>        }
>        blk_finish_plug(&plug);
> --
> 1.7.7.3
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Hillf Danton]

add lkml
On Tue, Apr 10, 2012 at 11:00 PM, Hillf Danton <dhillf@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 3:42 AM, Ying Han <yinghan@google.com> wrote:
>> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
>>
>> Before the commit, the code makes senses to me but not after the commit. The
>> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
>> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
>> example, we like to early break the reclaim if reclaimed 32 pages under direct
>> reclaim (not DEF_PRIORITY).
>>
>> After the reverted commit, the target "nr_to_reclaim" is decremented each time
>> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
>> doesn't make sense to me...
>>
> I downloaded mm/vmscan.c from the next tree a couple minutes ago, and
> see
>                .nr_to_reclaim = SWAP_CLUSTER_MAX,
> and
>        nr_reclaimed = do_try_to_free_pages(zonelist, &sc, &shrink);
>
> in try_to_free_pages().
>
> I also see
>                total_scanned += sc->nr_scanned;
>                if (sc->nr_reclaimed >= sc->nr_to_reclaim)
>                        goto out;
>
> in do_try_to_free_pages(),
>
> then would you please say a few words about the sense
> of the check of nr_to_reclaim?
>
>
>> Signed-off-by: Ying Han <yinghan@google.com>
>> ---
>>  mm/vmscan.c |    7 +------
>>  1 files changed, 1 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 33c332b..1a51868 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2107,12 +2107,7 @@ restart:
>>                 * with multiple processes reclaiming pages, the total
>>                 * freeing target can get unreasonably large.
>>                 */
>> -               if (nr_reclaimed >= nr_to_reclaim)
>> -                       nr_to_reclaim = 0;
>> -               else
>> -                       nr_to_reclaim -= nr_reclaimed;
>> -
>> -               if (!nr_to_reclaim && priority < DEF_PRIORITY)
>> +               if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>>                        break;
>>        }
>>        blk_finish_plug(&plug);
>> --
>> 1.7.7.3
>>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Ying Han]

On Tue, Apr 10, 2012 at 8:00 AM, Hillf Danton <dhillf@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 3:42 AM, Ying Han <yinghan@google.com> wrote:
>> This reverts commit c38446cc65e1f2b3eb8630c53943b94c4f65f670.
>>
>> Before the commit, the code makes senses to me but not after the commit. The
>> "nr_reclaimed" is the number of pages reclaimed by scanning through the memcg's
>> lru lists. The "nr_to_reclaim" is the target value for the whole function. For
>> example, we like to early break the reclaim if reclaimed 32 pages under direct
>> reclaim (not DEF_PRIORITY).
>>
>> After the reverted commit, the target "nr_to_reclaim" is decremented each time
>> by "nr_reclaimed" but we still use it to compare the "nr_reclaimed". It just
>> doesn't make sense to me...
>>
> I downloaded mm/vmscan.c from the next tree a couple minutes ago, and
> see
>                .nr_to_reclaim = SWAP_CLUSTER_MAX,
> and
>        nr_reclaimed = do_try_to_free_pages(zonelist, &sc, &shrink);
>
> in try_to_free_pages().
>
> I also see
>                total_scanned += sc->nr_scanned;
>                if (sc->nr_reclaimed >= sc->nr_to_reclaim)
>                        goto out;


> in do_try_to_free_pages(),
>
> then would you please say a few words about the sense
> of the check of nr_to_reclaim?

There are two places where we do early break out in direct reclaim path.

1. For each priority loop after calling shrink_zones(), we check
(sc->nr_reclaimed >= sc->nr_to_reclaim)

2. For each memcg reclaim (shrink_mem_cgroup_zone) under
shrink_zone(), we check (nr_reclaimed >= nr_to_reclaim)

The second one says "if 32 (nr_to_reclaim) pages being reclaimed from
this memcg under high priority, break". This check is necessary here
to prevent over pressure each memcg under shrink_zone().

Regarding the reverted patch, it tries to convert the "nr_reclaimed"
to "total_reclaimed" for outer loop (restart). First of all, it
changes the logic by doing less work each time
should_continue_reclaim() is true. Second, the fix is simply broken by
decrementing nr_to_reclaim each time.

--Ying

>
>
>> Signed-off-by: Ying Han <yinghan@google.com>
>> ---
>>  mm/vmscan.c |    7 +------
>>  1 files changed, 1 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 33c332b..1a51868 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2107,12 +2107,7 @@ restart:
>>                 * with multiple processes reclaiming pages, the total
>>                 * freeing target can get unreasonably large.
>>                 */
>> -               if (nr_reclaimed >= nr_to_reclaim)
>> -                       nr_to_reclaim = 0;
>> -               else
>> -                       nr_to_reclaim -= nr_reclaimed;
>> -
>> -               if (!nr_to_reclaim && priority < DEF_PRIORITY)
>> +               if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
>>                        break;
>>        }
>>        blk_finish_plug(&plug);
>> --
>> 1.7.7.3
>>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Revert "mm: vmscan: fix misused nr_reclaimed in shrink_mem_cgroup_zone()"

[Hillf Danton]

On Wed, Apr 11, 2012 at 12:44 AM, Ying Han <yinghan@google.com> wrote:
>
> There are two places where we do early break out in direct reclaim path.
>
> 1. For each priority loop after calling shrink_zones(), we check
> (sc->nr_reclaimed >= sc->nr_to_reclaim)
>
> 2. For each memcg reclaim (shrink_mem_cgroup_zone) under
> shrink_zone(), we check (nr_reclaimed >= nr_to_reclaim)
>
> The second one says "if 32 (nr_to_reclaim) pages being reclaimed from
> this memcg under high priority, break". This check is necessary here
> to prevent over pressure each memcg under shrink_zone().
>
> Regarding the reverted patch, it tries to convert the "nr_reclaimed"
> to "total_reclaimed" for outer loop (restart). First of all, it
> changes the logic by doing less work each time
> should_continue_reclaim() is true. Second, the fix is simply broken by
> decrementing nr_to_reclaim each time.
>
Got, thanks:)

-hd

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>