[PATCH/RFC] mm: do not regard CMA pages as free on watermark check

[PATCH/RFC] mm: do not regard CMA pages as free on watermark check

[Vitaly Wool]

__zone_watermark_ok() does not corrrectly take high-order
CMA pageblocks into account: high-order CMA blocks are not
removed from the watermark check. Moreover, CMA pageblocks
may suddenly vanish through CMA allocation, so let's not
regard these pages as free in __zone_watermark_ok().

This patch also adds some primitive testing for the method
implemented which has proven that it works as it should.

Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
---
 include/linux/mmzone.h |  1 +
 mm/page_alloc.c        | 56 +++++++++++++++++++++++++++++++++++++++++++++-----
 mm/page_isolation.c    |  2 +-
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index ac00e20..73268f5 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -92,6 +92,7 @@ static inline int get_pfnblock_migratetype(struct page *page, unsigned long pfn)
 struct free_area {
 	struct list_head	free_list[MIGRATE_TYPES];
 	unsigned long		nr_free;
+	unsigned long		nr_free_cma;
 };
 
 struct pglist_data;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5b5240b..69fbc93 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -672,6 +672,8 @@ static inline void __free_one_page(struct page *page,
 		} else {
 			list_del(&buddy->lru);
 			zone->free_area[order].nr_free--;
+			if (is_migrate_cma(migratetype))
+				zone->free_area[order].nr_free_cma--;
 			rmv_page_order(buddy);
 		}
 		combined_idx = buddy_idx & page_idx;
@@ -705,6 +707,8 @@ static inline void __free_one_page(struct page *page,
 	list_add(&page->lru, &zone->free_area[order].free_list[migratetype]);
 out:
 	zone->free_area[order].nr_free++;
+	if (is_migrate_cma(migratetype))
+		zone->free_area[order].nr_free_cma++;
 }
 
 static inline int free_pages_check(struct page *page)
@@ -1278,6 +1282,8 @@ static inline void expand(struct zone *zone, struct page *page,
 		}
 		list_add(&page[size].lru, &area->free_list[migratetype]);
 		area->nr_free++;
+		if (is_migrate_cma(migratetype))
+			area->nr_free_cma++;
 		set_page_order(&page[size], high);
 	}
 }
@@ -1379,6 +1385,8 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 		list_del(&page->lru);
 		rmv_page_order(page);
 		area->nr_free--;
+		if (is_migrate_cma(migratetype))
+			area->nr_free_cma--;
 		expand(zone, page, order, current_order, area, migratetype);
 		set_freepage_migratetype(page, migratetype);
 		return page;
@@ -1428,6 +1436,7 @@ int move_freepages(struct zone *zone,
 	struct page *page;
 	unsigned long order;
 	int pages_moved = 0;
+	int mt;
 
 #ifndef CONFIG_HOLES_IN_ZONE
 	/*
@@ -1457,7 +1466,12 @@ int move_freepages(struct zone *zone,
 		order = page_order(page);
 		list_move(&page->lru,
 			  &zone->free_area[order].free_list[migratetype]);
+		mt = get_pageblock_migratetype(page);
+		if (is_migrate_cma(mt))
+			zone->free_area[order].nr_free_cma--;
 		set_freepage_migratetype(page, migratetype);
+		if (is_migrate_cma(migratetype))
+			zone->free_area[order].nr_free_cma++;
 		page += 1 << order;
 		pages_moved += 1 << order;
 	}
@@ -1621,6 +1635,8 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 
 		/* Remove the page from the freelists */
 		area->nr_free--;
+		if (unlikely(is_migrate_cma(start_migratetype)))
+			area->nr_free_cma--;
 		list_del(&page->lru);
 		rmv_page_order(page);
 
@@ -2012,6 +2028,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
 	/* Remove page from free list */
 	list_del(&page->lru);
 	zone->free_area[order].nr_free--;
+	if (is_migrate_cma(mt))
+		zone->free_area[order].nr_free_cma--;
 	rmv_page_order(page);
 
 	set_page_owner(page, order, __GFP_MOVABLE);
@@ -2220,7 +2238,6 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
 	/* free_pages may go negative - that's OK */
 	long min = mark;
 	int o;
-	long free_cma = 0;
 
 	free_pages -= (1 << order) - 1;
 	if (alloc_flags & ALLOC_HIGH)
@@ -2228,17 +2245,43 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
 	if (alloc_flags & ALLOC_HARDER)
 		min -= min / 4;
 #ifdef CONFIG_CMA
-	/* If allocation can't use CMA areas don't use free CMA pages */
+	/*
+	 * We don't want to regard the pages on CMA region as free
+	 * on watermark checking, since they cannot be used for
+	 * unmovable/reclaimable allocation and they can suddenly
+	 * vanish through CMA allocation
+	 */
 	if (!(alloc_flags & ALLOC_CMA))
-		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
+		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
+#ifdef CONFIG_DEBUG_PAGEALLOC
+	{
+		long nr_free_cma;
+		for (o = 0, nr_free_cma = 0; o < MAX_ORDER; o++)
+			nr_free_cma += z->free_area[o].nr_free_cma << o;
+
+		/* nr_free_cma is a bit more realtime than zone_page_state
+		 * and may thus differ from it a little, and it's ok
+		 */
+		if (abs(nr_free_cma -
+				zone_page_state(z, NR_FREE_CMA_PAGES)) > 256)
+			pr_info_ratelimited("%s: nr_free_cma %ld instead of %ld\n",
+					__func__,
+					nr_free_cma,
+					zone_page_state(z, NR_FREE_CMA_PAGES));
+	}

-- 
Vitaly Wool <vitalywool@gmail.com>

Re: [PATCH/RFC] mm: do not regard CMA pages as free on watermark check

[Laura Abbott]

(cc-ing linux-mm)
On 09/09/2015 07:44 AM, Vitaly Wool wrote:
> __zone_watermark_ok() does not corrrectly take high-order
> CMA pageblocks into account: high-order CMA blocks are not
> removed from the watermark check. Moreover, CMA pageblocks
> may suddenly vanish through CMA allocation, so let's not
> regard these pages as free in __zone_watermark_ok().
>
> This patch also adds some primitive testing for the method
> implemented which has proven that it works as it should.
>

The choice to include CMA as part of watermarks was pretty deliberate.
Do you have a description of the problem you are facing with
the watermark code as is? Any performance numbers?

> Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
> ---
>   include/linux/mmzone.h |  1 +
>   mm/page_alloc.c        | 56 +++++++++++++++++++++++++++++++++++++++++++++-----
>   mm/page_isolation.c    |  2 +-
>   3 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index ac00e20..73268f5 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -92,6 +92,7 @@ static inline int get_pfnblock_migratetype(struct page *page, unsigned long pfn)
>   struct free_area {
>   	struct list_head	free_list[MIGRATE_TYPES];
>   	unsigned long		nr_free;
> +	unsigned long		nr_free_cma;
>   };
>
>   struct pglist_data;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5b5240b..69fbc93 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -672,6 +672,8 @@ static inline void __free_one_page(struct page *page,
>   		} else {
>   			list_del(&buddy->lru);
>   			zone->free_area[order].nr_free--;
> +			if (is_migrate_cma(migratetype))
> +				zone->free_area[order].nr_free_cma--;
>   			rmv_page_order(buddy);
>   		}
>   		combined_idx = buddy_idx & page_idx;
> @@ -705,6 +707,8 @@ static inline void __free_one_page(struct page *page,
>   	list_add(&page->lru, &zone->free_area[order].free_list[migratetype]);
>   out:
>   	zone->free_area[order].nr_free++;
> +	if (is_migrate_cma(migratetype))
> +		zone->free_area[order].nr_free_cma++;
>   }
>
>   static inline int free_pages_check(struct page *page)
> @@ -1278,6 +1282,8 @@ static inline void expand(struct zone *zone, struct page *page,
>   		}
>   		list_add(&page[size].lru, &area->free_list[migratetype]);
>   		area->nr_free++;
> +		if (is_migrate_cma(migratetype))
> +			area->nr_free_cma++;
>   		set_page_order(&page[size], high);
>   	}
>   }
> @@ -1379,6 +1385,8 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>   		list_del(&page->lru);
>   		rmv_page_order(page);
>   		area->nr_free--;
> +		if (is_migrate_cma(migratetype))
> +			area->nr_free_cma--;
>   		expand(zone, page, order, current_order, area, migratetype);
>   		set_freepage_migratetype(page, migratetype);
>   		return page;
> @@ -1428,6 +1436,7 @@ int move_freepages(struct zone *zone,
>   	struct page *page;
>   	unsigned long order;
>   	int pages_moved = 0;
> +	int mt;
>
>   #ifndef CONFIG_HOLES_IN_ZONE
>   	/*
> @@ -1457,7 +1466,12 @@ int move_freepages(struct zone *zone,
>   		order = page_order(page);
>   		list_move(&page->lru,
>   			  &zone->free_area[order].free_list[migratetype]);
> +		mt = get_pageblock_migratetype(page);
> +		if (is_migrate_cma(mt))
> +			zone->free_area[order].nr_free_cma--;
>   		set_freepage_migratetype(page, migratetype);
> +		if (is_migrate_cma(migratetype))
> +			zone->free_area[order].nr_free_cma++;
>   		page += 1 << order;
>   		pages_moved += 1 << order;
>   	}
> @@ -1621,6 +1635,8 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>
>   		/* Remove the page from the freelists */
>   		area->nr_free--;
> +		if (unlikely(is_migrate_cma(start_migratetype)))
> +			area->nr_free_cma--;
>   		list_del(&page->lru);
>   		rmv_page_order(page);
>
> @@ -2012,6 +2028,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
>   	/* Remove page from free list */
>   	list_del(&page->lru);
>   	zone->free_area[order].nr_free--;
> +	if (is_migrate_cma(mt))
> +		zone->free_area[order].nr_free_cma--;
>   	rmv_page_order(page);
>
>   	set_page_owner(page, order, __GFP_MOVABLE);
> @@ -2220,7 +2238,6 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>   	/* free_pages may go negative - that's OK */
>   	long min = mark;
>   	int o;
> -	long free_cma = 0;
>
>   	free_pages -= (1 << order) - 1;
>   	if (alloc_flags & ALLOC_HIGH)
> @@ -2228,17 +2245,43 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>   	if (alloc_flags & ALLOC_HARDER)
>   		min -= min / 4;
>   #ifdef CONFIG_CMA
> -	/* If allocation can't use CMA areas don't use free CMA pages */
> +	/*
> +	 * We don't want to regard the pages on CMA region as free
> +	 * on watermark checking, since they cannot be used for
> +	 * unmovable/reclaimable allocation and they can suddenly
> +	 * vanish through CMA allocation
> +	 */
>   	if (!(alloc_flags & ALLOC_CMA))
> -		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
> +		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> +	{
> +		long nr_free_cma;
> +		for (o = 0, nr_free_cma = 0; o < MAX_ORDER; o++)
> +			nr_free_cma += z->free_area[o].nr_free_cma << o;
> +
> +		/* nr_free_cma is a bit more realtime than zone_page_state
> +		 * and may thus differ from it a little, and it's ok
> +		 */
> +		if (abs(nr_free_cma -
> +				zone_page_state(z, NR_FREE_CMA_PAGES)) > 256)
> +			pr_info_ratelimited("%s: nr_free_cma %ld instead of %ld\n",
> +					__func__,
> +					nr_free_cma,
> +					zone_page_state(z, NR_FREE_CMA_PAGES));
> +	}
>

Re: [PATCH/RFC] mm: do not regard CMA pages as free on watermark check

[Vlastimil Babka]

[CC Joonsoo, Mel]

On 09/09/2015 08:31 PM, Vitaly Wool wrote:
> Hi Laura,
> 
> On Wed, Sep 9, 2015 at 7:56 PM, Laura Abbott <labbott@redhat.com> wrote:
> 
>> (cc-ing linux-mm)
>> On 09/09/2015 07:44 AM, Vitaly Wool wrote:
>>
>>> __zone_watermark_ok() does not corrrectly take high-order
>>> CMA pageblocks into account: high-order CMA blocks are not
>>> removed from the watermark check. Moreover, CMA pageblocks
>>> may suddenly vanish through CMA allocation, so let's not
>>> regard these pages as free in __zone_watermark_ok().
>>>
>>> This patch also adds some primitive testing for the method
>>> implemented which has proven that it works as it should.
>>>
>>>
>> The choice to include CMA as part of watermarks was pretty deliberate.
>> Do you have a description of the problem you are facing with
>> the watermark code as is? Any performance numbers?
>>
>>
> let's start with facing the fact that the calculation in
> __zone_watermark_ok() is done incorrectly for the case when ALLOC_CMA is
> not set. While going through pages by order it is implicitly considered

You're not the first who tried to fix it, I think Joonsoo tried as well?
I think the main objection was against further polluting fastpaths due to CMA.

Note that Mel has a patchset removing high-order watermark checks (in the last
patch of https://lwn.net/Articles/655406/ ) so this will be moot afterwards.

> that CMA pages can be used and this impacts the result of the function.
> 
> This can be solved in a slightly different way compared to what I proposed
> but it needs per-order CMA pages accounting anyway. Then it would have
> looked like:
> 
>         for (o = 0; o < order; o++) {
>                 /* At the next order, this order's pages become unavailable
> */
>                 free_pages -= z->free_area[o].nr_free << o;
> #ifdef CONFIG_CMA
>                 if (!(alloc_flags & ALLOC_CMA))
>                         free_pages -= z->free_area[o].nr_free_cma << o;
>                 /* Require fewer higher order pages to be free */
>                 min >>= 1;
> ...
> 
> But what we have also seen is that CMA pages may suddenly disappear due to
> CMA allocator work so the whole watermark checking was still unreliable,
> causing compaction to not run when it ought to and thus leading to

Well, watermark checking is inherently racy. CMA pages disappearing is no
exception, non-CMA pages may disappear as well.

> (otherwise redundant) low memory killer operations, so I decided to propose
> a safer method instead.
> 
> Best regards,
>    Vitaly
>