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

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));
> +	}
>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vitaly Wool]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

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
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
(otherwise redundant) low memory killer operations, so I decided to propose
a safer method instead.

Best regards,
   Vitaly

[-- Attachment #2: Type: text/html, Size: 2930 bytes --]

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
> 

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vitaly Wool]

[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]

On Thu, Sep 10, 2015 at 8:39 AM, Vlastimil Babka <vbabka@suse.cz> wrote:

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

I believe Joonsoo was calculating free_pages incorrectly, too, but in a
different way: he was subtracting CMA pages twice.


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

I am not quite convinced that nested loops are a better solution than what
I suggest.


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

Right, that is why I decided to play on the safe side.

 ~vitaly

[-- Attachment #2: Type: text/html, Size: 4187 bytes --]