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

[Huang Shijie <b32955@freescale.com>]

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>