Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463436 > unrolled thread

Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:20 +0200
    Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:40 +0200
      Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:50 +0200
        Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:30 +0200

#1463436 — Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders

On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
> The __compaction_suitable() function checks the low watermark plus a
> compact_gap() gap to decide if there's enough free memory to perform
> compaction. Then __isolate_free_page uses low watermark check to decide if
> particular free page can be isolated. In the latter case, using low watermark
> is needlessly pessimistic, as the free page isolations are only temporary. For
> __compaction_suitable() the higher watermark makes sense for high-order
> allocations where more freepages increase the chance of success, and we can
> typically fail with some order-0 fallback when the system is struggling to
> reach that watermark. But for low-order allocation, forming the page should not
> be that hard. So using low watermark here might just prevent compaction from
> even trying, and eventually lead to OOM killer even if we are above min
> watermarks.
> 
> So after this patch, we use min watermark for non-costly orders in
> __compaction_suitable(), and for all orders in __isolate_free_page().
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/compaction.c | 6 +++++-
>  mm/page_alloc.c | 2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 80eaf9fff114..0bba270f97ad 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1399,10 +1399,14 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
>  	 * isolation. We however do use the direct compactor's classzone_idx to
>  	 * skip over zones where lowmem reserves would prevent allocation even
>  	 * if compaction succeeds.
> +	 * For costly orders, we require low watermark instead of min for
> +	 * compaction to proceed to increase its chances.
>  	 * ALLOC_CMA is used, as pages in CMA pageblocks are considered
>  	 * suitable migration targets
>  	 */
> -	watermark = low_wmark_pages(zone) + compact_gap(order);
> +	watermark = (order > PAGE_ALLOC_COSTLY_ORDER) ?
> +				low_wmark_pages(zone) : min_wmark_pages(zone);
> +	watermark += compact_gap(order);
>  	if (!__zone_watermark_ok(zone, 0, watermark, classzone_idx,
>  						ALLOC_CMA, wmark_target))
>  		return COMPACT_SKIPPED;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 621e4211ce16..a5c0f914ec00 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  
>  	if (!is_migrate_isolate(mt)) {
>  		/* Obey watermarks as if the page was being allocated */
> -		watermark = low_wmark_pages(zone) + (1 << order);
> +		watermark = min_wmark_pages(zone) + (1UL << order);

This '1 << order' also needs some comment. Why can't we use
compact_gap() in this case?

Thanks.

[toc] | [next] | [standalone]

#1463454

On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
> On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 621e4211ce16..a5c0f914ec00 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>>
>>  	if (!is_migrate_isolate(mt)) {
>>  		/* Obey watermarks as if the page was being allocated */
>> -		watermark = low_wmark_pages(zone) + (1 << order);
>> +		watermark = min_wmark_pages(zone) + (1UL << order);
>
> This '1 << order' also needs some comment. Why can't we use
> compact_gap() in this case?

This is just short-cutting the high-order watermark check to check only 
order-0, because we already know the high-order page exists.
We can't use compact_gap() as that's too high to use for a single 
allocation watermark, since we can be already holding some free pages on 
the list. So it would defeat the gap purpose.

> Thanks.
>

[toc] | [prev] | [next] | [standalone]

#1463458

On Tue, Aug 16, 2016 at 08:36:12AM +0200, Vlastimil Babka wrote:
> On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
> >On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
> >>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >>index 621e4211ce16..a5c0f914ec00 100644
> >>--- a/mm/page_alloc.c
> >>+++ b/mm/page_alloc.c
> >>@@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
> >>
> >> 	if (!is_migrate_isolate(mt)) {
> >> 		/* Obey watermarks as if the page was being allocated */
> >>-		watermark = low_wmark_pages(zone) + (1 << order);
> >>+		watermark = min_wmark_pages(zone) + (1UL << order);
> >
> >This '1 << order' also needs some comment. Why can't we use
> >compact_gap() in this case?
> 
> This is just short-cutting the high-order watermark check to check
> only order-0, because we already know the high-order page exists.
> We can't use compact_gap() as that's too high to use for a single
> allocation watermark, since we can be already holding some free
> pages on the list. So it would defeat the gap purpose.

Oops. I missed that. Thanks for clarifying it.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1465222

On 08/16/2016 08:46 AM, Joonsoo Kim wrote:
> On Tue, Aug 16, 2016 at 08:36:12AM +0200, Vlastimil Babka wrote:
>> On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
>>> On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index 621e4211ce16..a5c0f914ec00 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>>>>
>>>> 	if (!is_migrate_isolate(mt)) {
>>>> 		/* Obey watermarks as if the page was being allocated */
>>>> -		watermark = low_wmark_pages(zone) + (1 << order);
>>>> +		watermark = min_wmark_pages(zone) + (1UL << order);
>>>
>>> This '1 << order' also needs some comment. Why can't we use
>>> compact_gap() in this case?
>>
>> This is just short-cutting the high-order watermark check to check
>> only order-0, because we already know the high-order page exists.
>> We can't use compact_gap() as that's too high to use for a single
>> allocation watermark, since we can be already holding some free
>> pages on the list. So it would defeat the gap purpose.
> 
> Oops. I missed that. Thanks for clarifying it.

So let's expand the comment?

----8<----
From 5d060f4222a637e1005ff32ae0fd4330625b6675 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 18 Aug 2016 14:18:08 +0200
Subject: [PATCH] mm, compaction: require only min watermarks for non-costly
 orders-fix

Clarify why __isolate_free_page() does a order-0 watermark check with
apparent (1UL << order) gap, per Joonsoo.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a5c0f914ec00..216715504fb4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2491,7 +2491,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
 	mt = get_pageblock_migratetype(page);
 
 	if (!is_migrate_isolate(mt)) {
-		/* Obey watermarks as if the page was being allocated */
+		/*
+		 * Obey watermarks as if the page was being allocated. We can
+		 * emulate a high-order watermark check with a raised order-0
+		 * watermark, because we already know our high-order page
+		 * exists.
+		 */
 		watermark = min_wmark_pages(zone) + (1UL << order);
 		if (!zone_watermark_ok(zone, 0, watermark, 0, ALLOC_CMA))
 			return 0;
-- 
2.9.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web