[PATCH v2] mm/compaction : check the watermark when cc->order is -1

[PATCH v2] mm/compaction : check the watermark when cc->order is -1

[Huang Shijie]

We get cc->order is -1 when user echos to /proc/sys/vm/compact_memory.
In this case, we should check that if we have enough pages for
the compaction in the zone.

If we do not check this, in our MX6Q board(arm), i ever observed
COMPACT_CLUSTER_MAX pages were compaction failed in per migrate_pages().
Thats mean we can not alloc any pages by the free scanner in the zone.

This patch checks the watermark to avoid this problem.
Tested this patch in the MX6Q board.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 mm/compaction.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 899d956..bf8e8b2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -479,21 +479,21 @@ unsigned long compaction_suitable(struct zone *zone, int order)
 	unsigned long watermark;
 
 	/*
+	 * Watermarks for order-0 must be met for compaction.
+	 * During the migration, copies of pages need to be
+	 * allocated and for a short time, so the footprint is higher.
 	 * order == -1 is expected when compacting via
-	 * /proc/sys/vm/compact_memory
+	 * /proc/sys/vm/compact_memory.
 	 */
-	if (order == -1)
-		return COMPACT_CONTINUE;
+	watermark = low_wmark_pages(zone) +
+		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL << order));
 
-	/*
-	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
-	 * This is because during migration, copies of pages need to be
-	 * allocated and for a short time, the footprint is higher
-	 */
-	watermark = low_wmark_pages(zone) + (2UL << order);
 	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
 		return COMPACT_SKIPPED;
 
+	if (order == -1)
+		return COMPACT_CONTINUE;
+
 	/*
 	 * fragmentation index determines if allocation failures are due to
 	 * low memory or external fragmentation
-- 
1.7.3.2


--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Huang Shijie]

于 2012年01月06日 10:50, Huang Shijie 写道:
> We get cc->order is -1 when user echos to /proc/sys/vm/compact_memory.
> In this case, we should check that if we have enough pages for
> the compaction in the zone.
>
> If we do not check this, in our MX6Q board(arm), i ever observed
> COMPACT_CLUSTER_MAX pages were compaction failed in per migrate_pages().
> Thats mean we can not alloc any pages by the free scanner in the zone.
>
> This patch checks the watermark to avoid this problem.
> Tested this patch in the MX6Q board.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  mm/compaction.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 899d956..bf8e8b2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -479,21 +479,21 @@ unsigned long compaction_suitable(struct zone *zone, int order)
>  	unsigned long watermark;
>  
>  	/*
> +	 * Watermarks for order-0 must be met for compaction.
> +	 * During the migration, copies of pages need to be
> +	 * allocated and for a short time, so the footprint is higher.
>  	 * order == -1 is expected when compacting via
> -	 * /proc/sys/vm/compact_memory
> +	 * /proc/sys/vm/compact_memory.
>  	 */
> -	if (order == -1)
> -		return COMPACT_CONTINUE;
> +	watermark = low_wmark_pages(zone) +
> +		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL << order));
>  
> -	/*
> -	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
> -	 * This is because during migration, copies of pages need to be
> -	 * allocated and for a short time, the footprint is higher
> -	 */
> -	watermark = low_wmark_pages(zone) + (2UL << order);
>  	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
>  		return COMPACT_SKIPPED;
>  
> +	if (order == -1)
> +		return COMPACT_CONTINUE;
> +
>  	/*
>  	 * fragmentation index determines if allocation failures are due to
>  	 * low memory or external fragmentation
Is this patch meaningless?
I really think this patch is useful when the zone is nearly full.

Best Regards

Huang Shijie

--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Minchan Kim]

On Fri, Jan 06, 2012 at 10:50:01AM +0800, Huang Shijie wrote:
> We get cc->order is -1 when user echos to /proc/sys/vm/compact_memory.
> In this case, we should check that if we have enough pages for
> the compaction in the zone.
> 
> If we do not check this, in our MX6Q board(arm), i ever observed
> COMPACT_CLUSTER_MAX pages were compaction failed in per migrate_pages().
> Thats mean we can not alloc any pages by the free scanner in the zone.
> 

It seems this patch is useful but I can't parse your this sentense.
Could you elaborate on it?

We should know about exactly why order == -1 is COMPACT_CLUSTER_MAX * 2 and other case
2UL << order. If you write down description more clear, it will help.

Thanks.

> This patch checks the watermark to avoid this problem.
> Tested this patch in the MX6Q board.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  mm/compaction.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 899d956..bf8e8b2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -479,21 +479,21 @@ unsigned long compaction_suitable(struct zone *zone, int order)
>  	unsigned long watermark;
>  
>  	/*
> +	 * Watermarks for order-0 must be met for compaction.
> +	 * During the migration, copies of pages need to be
> +	 * allocated and for a short time, so the footprint is higher.
>  	 * order == -1 is expected when compacting via
> -	 * /proc/sys/vm/compact_memory
> +	 * /proc/sys/vm/compact_memory.
>  	 */
> -	if (order == -1)
> -		return COMPACT_CONTINUE;
> +	watermark = low_wmark_pages(zone) +
> +		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL << order));
>  
> -	/*
> -	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
> -	 * This is because during migration, copies of pages need to be
> -	 * allocated and for a short time, the footprint is higher
> -	 */
> -	watermark = low_wmark_pages(zone) + (2UL << order);
>  	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
>  		return COMPACT_SKIPPED;
>  
> +	if (order == -1)
> +		return COMPACT_CONTINUE;
> +
>  	/*
>  	 * fragmentation index determines if allocation failures are due to
>  	 * low memory or external fragmentation
> -- 
> 1.7.3.2
> 
> 
> --
> 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>

--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Huang Shijie]

Hi,
> It seems this patch is useful but I can't parse your this sentense.
> Could you elaborate on it?
sorry for my poor english.
> We should know about exactly why order == -1 is COMPACT_CLUSTER_MAX * 2 and other case
> 2UL<<  order. If you write down description more clear, it will help.
>
ok.

thanks

Huang Shijie


--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Mel Gorman]

On Thu, Jan 12, 2012 at 01:59:26PM +0800, Huang Shijie wrote:
> ?? 2012??01??06?? 10:50, Huang Shijie ????:
> > We get cc->order is -1 when user echos to /proc/sys/vm/compact_memory.
> > In this case, we should check that if we have enough pages for
> > the compaction in the zone.
> >
> > If we do not check this, in our MX6Q board(arm), i ever observed
> > COMPACT_CLUSTER_MAX pages were compaction failed in per migrate_pages().
> > Thats mean we can not alloc any pages by the free scanner in the zone.
> >
> > This patch checks the watermark to avoid this problem.
> > Tested this patch in the MX6Q board.
> >
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  mm/compaction.c |   18 +++++++++---------
> >  1 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 899d956..bf8e8b2 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -479,21 +479,21 @@ unsigned long compaction_suitable(struct zone *zone, int order)
> >  	unsigned long watermark;
> >  
> >  	/*
> > +	 * Watermarks for order-0 must be met for compaction.
> > +	 * During the migration, copies of pages need to be
> > +	 * allocated and for a short time, so the footprint is higher.
> >  	 * order == -1 is expected when compacting via
> > -	 * /proc/sys/vm/compact_memory
> > +	 * /proc/sys/vm/compact_memory.
> >  	 */
> > -	if (order == -1)
> > -		return COMPACT_CONTINUE;
> > +	watermark = low_wmark_pages(zone) +
> > +		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL << order));
> >  
> > -	/*
> > -	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
> > -	 * This is because during migration, copies of pages need to be
> > -	 * allocated and for a short time, the footprint is higher
> > -	 */
> > -	watermark = low_wmark_pages(zone) + (2UL << order);
> >  	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
> >  		return COMPACT_SKIPPED;
> >  
> > +	if (order == -1)
> > +		return COMPACT_CONTINUE;
> > +
> >  	/*
> >  	 * fragmentation index determines if allocation failures are due to
> >  	 * low memory or external fragmentation
> Is this patch meaningless?
> I really think this patch is useful when the zone is nearly full.
> 

Code wise the patch is fine. One reason why it fell off my radar is
because you mangled the comments for no apparent reason. Specifically,
after your patch is applied the code looks like this

        /*
         * Watermarks for order-0 must be met for compaction.
         * During the migration, copies of pages need to be
         * allocated and for a short time, so the footprint is higher.
         * order == -1 is expected when compacting via
         * /proc/sys/vm/compact_memory.
         */
        watermark = low_wmark_pages(zone) +
                ((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL << order));

        if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
                return COMPACT_SKIPPED;

        if (order == -1)
                return COMPACT_CONTINUE;

The comment about "order == -1" is no longer with the code it refers
to. I did not get at the time why the patch was not

diff --git a/mm/compaction.c b/mm/compaction.c
index 899d956..c96139a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -479,13 +479,6 @@ unsigned long compaction_suitable(struct zone *zone, int order)
 	unsigned long watermark;
 
 	/*
-	 * order == -1 is expected when compacting via
-	 * /proc/sys/vm/compact_memory
-	 */
-	if (order == -1)
-		return COMPACT_CONTINUE;
-
-	/*
 	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
 	 * This is because during migration, copies of pages need to be
 	 * allocated and for a short time, the footprint is higher
@@ -495,6 +488,13 @@ unsigned long compaction_suitable(struct zone *zone, int order)
 		return COMPACT_SKIPPED;
 
 	/*
+	 * order == -1 is expected when compacting via
+	 * /proc/sys/vm/compact_memory
+	 */
+	if (order == -1)
+		return COMPACT_CONTINUE;
+
+	/*
 	 * fragmentation index determines if allocation failures are due to
 	 * low memory or external fragmentation
 	 *

Later I for about this patch in the midst of other bug investigations.

The changelog was also a bit rough but as the change should be fairly
straight forward, it did not concern me as much.

-- 
Mel Gorman
SUSE Labs

--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Huang Shijie]

Hi,
> On Thu, Jan 12, 2012 at 01:59:26PM +0800, Huang Shijie wrote:
>> ?? 2012??01??06?? 10:50, Huang Shijie ????:
>>> We get cc->order is -1 when user echos to /proc/sys/vm/compact_memory.
>>> In this case, we should check that if we have enough pages for
>>> the compaction in the zone.
>>>
>>> If we do not check this, in our MX6Q board(arm), i ever observed
>>> COMPACT_CLUSTER_MAX pages were compaction failed in per migrate_pages().
>>> Thats mean we can not alloc any pages by the free scanner in the zone.
>>>
>>> This patch checks the watermark to avoid this problem.
>>> Tested this patch in the MX6Q board.
>>>
>>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>>> ---
>>>   mm/compaction.c |   18 +++++++++---------
>>>   1 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 899d956..bf8e8b2 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -479,21 +479,21 @@ unsigned long compaction_suitable(struct zone *zone, int order)
>>>   	unsigned long watermark;
>>>
>>>   	/*
>>> +	 * Watermarks for order-0 must be met for compaction.
>>> +	 * During the migration, copies of pages need to be
>>> +	 * allocated and for a short time, so the footprint is higher.
>>>   	 * order == -1 is expected when compacting via
>>> -	 * /proc/sys/vm/compact_memory
>>> +	 * /proc/sys/vm/compact_memory.
>>>   	 */
>>> -	if (order == -1)
>>> -		return COMPACT_CONTINUE;
>>> +	watermark = low_wmark_pages(zone) +
>>> +		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<<  order));
>>>
>>> -	/*
>>> -	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
>>> -	 * This is because during migration, copies of pages need to be
>>> -	 * allocated and for a short time, the footprint is higher
>>> -	 */
>>> -	watermark = low_wmark_pages(zone) + (2UL<<  order);
>>>   	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
>>>   		return COMPACT_SKIPPED;
>>>
>>> +	if (order == -1)
>>> +		return COMPACT_CONTINUE;
>>> +
>>>   	/*
>>>   	 * fragmentation index determines if allocation failures are due to
>>>   	 * low memory or external fragmentation
>> Is this patch meaningless?
>> I really think this patch is useful when the zone is nearly full.
>>
> Code wise the patch is fine. One reason why it fell off my radar is
> because you mangled the comments for no apparent reason. Specifically,
> after your patch is applied the code looks like this
>
>          /*
>           * Watermarks for order-0 must be met for compaction.
>           * During the migration, copies of pages need to be
>           * allocated and for a short time, so the footprint is higher.
>           * order == -1 is expected when compacting via
>           * /proc/sys/vm/compact_memory.
>           */
>          watermark = low_wmark_pages(zone) +
>                  ((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<<  order));
"order == -1" first appears here.
>          if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
>                  return COMPACT_SKIPPED;
>
>          if (order == -1)
>                  return COMPACT_CONTINUE;
>
> The comment about "order == -1" is no longer with the code it refers
If I keep the comment here, someone may wonder why the `order == -1` 
firstly appears above.

I just want to keep the comment where it firstly appears. Don't you 
think it's right?

> to. I did not get at the time why the patch was not
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 899d956..c96139a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -479,13 +479,6 @@ unsigned long compaction_suitable(struct zone *zone, int order)
>   	unsigned long watermark;
>
>   	/*
> -	 * order == -1 is expected when compacting via
> -	 * /proc/sys/vm/compact_memory
> -	 */
> -	if (order == -1)
> -		return COMPACT_CONTINUE;
> -
> -	/*
>   	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
>   	 * This is because during migration, copies of pages need to be
>   	 * allocated and for a short time, the footprint is higher
> @@ -495,6 +488,13 @@ unsigned long compaction_suitable(struct zone *zone, int order)
>   		return COMPACT_SKIPPED;
>
>   	/*
> +	 * order == -1 is expected when compacting via
> +	 * /proc/sys/vm/compact_memory
> +	 */
> +	if (order == -1)
> +		return COMPACT_CONTINUE;
> +
> +	/*
>   	 * fragmentation index determines if allocation failures are due to
>   	 * low memory or external fragmentation
>   	 *
>
> Later I for about this patch in the midst of other bug investigations.
>
> The changelog was also a bit rough but as the change should be fairly
> straight forward, it did not concern me as much.
>
thanks. I will improve the log.

Huang Shijie


--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Mel Gorman]

On Fri, Jan 13, 2012 at 10:31:12AM +0800, Huang Shijie wrote:
> >>>  	/*
> >>>+	 * Watermarks for order-0 must be met for compaction.
> >>>+	 * During the migration, copies of pages need to be
> >>>+	 * allocated and for a short time, so the footprint is higher.
> >>>  	 * order == -1 is expected when compacting via
> >>>-	 * /proc/sys/vm/compact_memory
> >>>+	 * /proc/sys/vm/compact_memory.
> >>>  	 */
> >>>-	if (order == -1)
> >>>-		return COMPACT_CONTINUE;
> >>>+	watermark = low_wmark_pages(zone) +
> >>>+		((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<<  order));
> >>>
> >>>-	/*
> >>>-	 * Watermarks for order-0 must be met for compaction. Note the 2UL.
> >>>-	 * This is because during migration, copies of pages need to be
> >>>-	 * allocated and for a short time, the footprint is higher
> >>>-	 */
> >>>-	watermark = low_wmark_pages(zone) + (2UL<<  order);
> >>>  	if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
> >>>  		return COMPACT_SKIPPED;
> >>>
> >>>+	if (order == -1)
> >>>+		return COMPACT_CONTINUE;
> >>>+
> >>>  	/*
> >>>  	 * fragmentation index determines if allocation failures are due to
> >>>  	 * low memory or external fragmentation
> >>Is this patch meaningless?
> >>I really think this patch is useful when the zone is nearly full.
> >>
> >Code wise the patch is fine. One reason why it fell off my radar is
> >because you mangled the comments for no apparent reason. Specifically,
> >after your patch is applied the code looks like this
> >
> >         /*
> >          * Watermarks for order-0 must be met for compaction.
> >          * During the migration, copies of pages need to be
> >          * allocated and for a short time, so the footprint is higher.
> >          * order == -1 is expected when compacting via
> >          * /proc/sys/vm/compact_memory.
> >          */
> >         watermark = low_wmark_pages(zone) +
> >                 ((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<<  order));
> "order == -1" first appears here.
> >         if (!zone_watermark_ok(zone, 0, watermark, 0, 0))
> >                 return COMPACT_SKIPPED;
> >
> >         if (order == -1)
> >                 return COMPACT_CONTINUE;
> >
> >The comment about "order == -1" is no longer with the code it refers
> If I keep the comment here, someone may wonder why the `order == -1`
> firstly appears above.
> 
> I just want to keep the comment where it firstly appears. Don't you
> think it's right?
> 

Bah, I'm an idiot.

When I glanced at this first, I missed that you altered the watermark
check as well. When I said "Code wise the patch is fine", I was wrong.
Compaction works in units of pageblocks and the watermark check
is necessary. Reducing it to COMPACT_CLUSTER_MAX*2 leads to the
possibility of compaction via /proc causing livelocks in low memory
situations depending on the value of min_free_kbytes.

-- 
Mel Gorman
SUSE Labs

--
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 v2] mm/compaction : check the watermark when cc->order is -1

[Huang Shijie]

hi,
> When I glanced at this first, I missed that you altered the watermark
> check as well. When I said "Code wise the patch is fine", I was wrong.
> Compaction works in units of pageblocks and the watermark check
> is necessary. Reducing it to COMPACT_CLUSTER_MAX*2 leads to the
> possibility of compaction via /proc causing livelocks in low memory
> situations depending on the value of min_free_kbytes.

ok, thanks a lot for the explanation.

Huang Shijie

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