diff mbox series

[PATCHv2] mm: optimization on page allocation when CMA enabled

Message ID 1683194994-3070-1-git-send-email-zhaoyang.huang@unisoc.com
State New
Headers
Series [PATCHv2] mm: optimization on page allocation when CMA enabled |

Commit Message

zhaoyang.huang May 4, 2023, 10:09 a.m. UTC 
  From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

Let us look at the series of scenarios below with WMARK_LOW=25MB,WMARK_MIN=5MB
(managed pages 1.9GB). We can know that current 'fixed 1/2 ratio' start to use
CMA since C which actually has caused U&R lower than WMARK_LOW (this should be
deemed as against current memory policy, that is, U&R should either stay around
WATERMARK_LOW when no allocation or do reclaim via enter slowpath)

free_cma/free_pages(MB)      A(12/30)     B(12/25)     C(12/20)
fixed 1/2 ratio                 N             N           Y
this commit                     Y             Y           Y

Suggested-by: Roman Gushchin <roman.gushchin@linux.dev>
Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
v2: do proportion check when zone_watermark_ok, update commit message
---
---
 mm/page_alloc.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Comments

kernel test robot May 4, 2023, 4:48 p.m. UTC | #1 
Hi zhaoyang.huang,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/zhaoyang-huang/mm-optimization-on-page-allocation-when-CMA-enabled/20230504-181335
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/1683194994-3070-1-git-send-email-zhaoyang.huang%40unisoc.com
patch subject: [PATCHv2] mm: optimization on page allocation when CMA enabled
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20230505/202305050012.G279ml2k-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/46cd0a3d98d6b43cd59be9d9e743266fc7f61168
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review zhaoyang-huang/mm-optimization-on-page-allocation-when-CMA-enabled/20230504-181335
        git checkout 46cd0a3d98d6b43cd59be9d9e743266fc7f61168
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305050012.G279ml2k-lkp@intel.com/

All errors (new ones prefixed by >>):

   mm/page_alloc.c: In function '__rmqueue':
>> mm/page_alloc.c:2323:42: error: implicit declaration of function '__if_use_cma_first' [-Werror=implicit-function-declaration]
    2323 |                         bool cma_first = __if_use_cma_first(zone, order, alloc_flags);
         |                                          ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/__if_use_cma_first +2323 mm/page_alloc.c

  2277	
  2278	#ifdef CONFIG_CMA
  2279	static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)
  2280	{
  2281		unsigned long cma_proportion = 0;
  2282		unsigned long cma_free_proportion = 0;
  2283		unsigned long watermark = 0;
  2284		long count = 0;
  2285		bool cma_first = false;
  2286	
  2287		watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
  2288		/*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/
  2289		if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
  2290			/* WMARK_LOW failed lead to using cma first, this helps U&R stay
  2291			 * around low when being drained by GFP_MOVABLE
  2292			 */
  2293			cma_first = true;
  2294		else {
  2295			/*check proportion when zone_watermark_ok*/
  2296			count = atomic_long_read(&zone->managed_pages);
  2297			cma_proportion = zone->cma_pages * 100 / count;
  2298			cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
  2299				/  zone_page_state(zone, NR_FREE_PAGES);
  2300			cma_first = (cma_free_proportion >= cma_proportion * 2
  2301					|| cma_free_proportion >= 50);
  2302		}
  2303		return cma_first;
  2304	}
  2305	#endif
  2306	/*
  2307	 * Do the hard work of removing an element from the buddy allocator.
  2308	 * Call me with the zone->lock already held.
  2309	 */
  2310	static __always_inline struct page *
  2311	__rmqueue(struct zone *zone, unsigned int order, int migratetype,
  2312							unsigned int alloc_flags)
  2313	{
  2314		struct page *page;
  2315	
  2316		if (IS_ENABLED(CONFIG_CMA)) {
  2317			/*
  2318			 * Balance movable allocations between regular and CMA areas by
  2319			 * allocating from CMA when over half of the zone's free memory
  2320			 * is in the CMA area.
  2321			 */
  2322			if (migratetype == MIGRATE_MOVABLE) {
> 2323				bool cma_first = __if_use_cma_first(zone, order, alloc_flags);
  2324	
  2325				page = cma_first ? __rmqueue_cma_fallback(zone, order) : NULL;
  2326				if (page)
  2327					return page;
  2328			}
  2329		}
  2330	retry:
  2331		page = __rmqueue_smallest(zone, order, migratetype);
  2332		if (unlikely(!page)) {
  2333			if (alloc_flags & ALLOC_CMA)
  2334				page = __rmqueue_cma_fallback(zone, order);
  2335	
  2336			if (!page && __rmqueue_fallback(zone, order, migratetype,
  2337									alloc_flags))
  2338				goto retry;
  2339		}
  2340		return page;
  2341	}
  2342
Zhaoyang Huang May 5, 2023, 8:02 a.m. UTC | #2 
add more reviewer

On Thu, May 4, 2023 at 6:11 PM zhaoyang.huang <zhaoyang.huang@unisoc.com> wrote:
>
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
>
> Let us look at the series of scenarios below with WMARK_LOW=25MB,WMARK_MIN=5MB
> (managed pages 1.9GB). We can know that current 'fixed 1/2 ratio' start to use
> CMA since C which actually has caused U&R lower than WMARK_LOW (this should be
> deemed as against current memory policy, that is, U&R should either stay around
> WATERMARK_LOW when no allocation or do reclaim via enter slowpath)
>
> free_cma/free_pages(MB)      A(12/30)     B(12/25)     C(12/20)
> fixed 1/2 ratio                 N             N           Y
> this commit                     Y             Y           Y
>
> Suggested-by: Roman Gushchin <roman.gushchin@linux.dev>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
> v2: do proportion check when zone_watermark_ok, update commit message
> ---
> ---
>  mm/page_alloc.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0745aed..d0baeab 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3071,6 +3071,34 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>
>  }
>
> +#ifdef CONFIG_CMA
> +static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)
> +{
> +       unsigned long cma_proportion = 0;
> +       unsigned long cma_free_proportion = 0;
> +       unsigned long watermark = 0;
> +       long count = 0;
> +       bool cma_first = false;
> +
> +       watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> +       /*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/
> +       if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
> +               /* WMARK_LOW failed lead to using cma first, this helps U&R stay
> +                * around low when being drained by GFP_MOVABLE
> +                */
> +               cma_first = true;
> +       else {
> +               /*check proportion when zone_watermark_ok*/
> +               count = atomic_long_read(&zone->managed_pages);
> +               cma_proportion = zone->cma_pages * 100 / count;
> +               cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
> +                       /  zone_page_state(zone, NR_FREE_PAGES);
> +               cma_first = (cma_free_proportion >= cma_proportion * 2
> +                               || cma_free_proportion >= 50);
> +       }
> +       return cma_first;
> +}
> +#endif
>  /*
>   * Do the hard work of removing an element from the buddy allocator.
>   * Call me with the zone->lock already held.
> @@ -3087,10 +3115,10 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>                  * allocating from CMA when over half of the zone's free memory
>                  * is in the CMA area.
>                  */
> -               if (alloc_flags & ALLOC_CMA &&
> -                   zone_page_state(zone, NR_FREE_CMA_PAGES) >
> -                   zone_page_state(zone, NR_FREE_PAGES) / 2) {
> -                       page = __rmqueue_cma_fallback(zone, order);
> +               if (migratetype == MIGRATE_MOVABLE) {
> +                       bool cma_first = __if_use_cma_first(zone, order, alloc_flags);
> +
> +                       page = cma_first ? __rmqueue_cma_fallback(zone, order) : NULL;
>                         if (page)
>                                 return page;
>                 }
> --
> 1.9.1
>
Andrew Morton May 5, 2023, 9:25 p.m. UTC | #3 
On Thu, 4 May 2023 18:09:54 +0800 "zhaoyang.huang" <zhaoyang.huang@unisoc.com> wrote:

> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Let us look at the series of scenarios below with WMARK_LOW=25MB,WMARK_MIN=5MB
> (managed pages 1.9GB). We can know that current 'fixed 1/2 ratio' start to use
> CMA since C which actually has caused U&R lower than WMARK_LOW (this should be
> deemed as against current memory policy, that is, U&R should either stay around
> WATERMARK_LOW when no allocation or do reclaim via enter slowpath)
> 
> free_cma/free_pages(MB)      A(12/30)     B(12/25)     C(12/20)
> fixed 1/2 ratio                 N             N           Y
> this commit                     Y             Y           Y

A few style issues.

> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3071,6 +3071,34 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>  
>  }
>  
> +#ifdef CONFIG_CMA
> +static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)

This function would benefit from a nice covering comment.  Explain what
it does and, especially, why it does it.

I'm not sure that "__if_use_cma_first" is a good name - I'd need to see
that explanation to decide.

> +{
> +	unsigned long cma_proportion = 0;
> +	unsigned long cma_free_proportion = 0;
> +	unsigned long watermark = 0;
> +	long count = 0;
> +	bool cma_first = false;

We seems to have some unnecessary initializations here.

> +	watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> +	/*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/

Space after /* and before */

	/*
	 * Check if GFP_MOVABLE passed the previous watermark check with the
	 * help of CMA
	 */

> +	if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
> +		/* WMARK_LOW failed lead to using cma first, this helps U&R stay
> +		 * around low when being drained by GFP_MOVABLE
> +		 */

Unusual layout, text is hard to understand.  Maybe something like

		/*
		 * WMARK_LOW failed, leading to the use cma first.  This helps
		 * U&R stay low when <something> is being drained by
		 * GFP_MOVABLE
		 */

Also, please expand "U&R" into full words.  I don't recognize that
abbreviation.

> +		cma_first = true;
> +	else {
> +		/*check proportion when zone_watermark_ok*/

		/* check ... _ok */

Comments should seek to explain *why* a thing is being done, rather than
*what* is being done.

> +		count = atomic_long_read(&zone->managed_pages);
> +		cma_proportion = zone->cma_pages * 100 / count;
> +		cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
> +			/  zone_page_state(zone, NR_FREE_PAGES);
> +		cma_first = (cma_free_proportion >= cma_proportion * 2
> +				|| cma_free_proportion >= 50);
> +	}
> +	return cma_first;
> +}
> +#endif
>  /*
>   * Do the hard work of removing an element from the buddy allocator.
>   * Call me with the zone->lock already held.
> @@ -3087,10 +3115,10 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,

I wonder why git decided this hunk is unreserve_highatomic_pageblock().
It's actually in __rmqueue().

>  		 * allocating from CMA when over half of the zone's free memory
>  		 * is in the CMA area.
>  		 */
> -		if (alloc_flags & ALLOC_CMA &&
> -		    zone_page_state(zone, NR_FREE_CMA_PAGES) >
> -		    zone_page_state(zone, NR_FREE_PAGES) / 2) {
> -			page = __rmqueue_cma_fallback(zone, order);
> +		if (migratetype == MIGRATE_MOVABLE) {
> +			bool cma_first = __if_use_cma_first(zone, order, alloc_flags);
> +
> +			page = cma_first ? __rmqueue_cma_fallback(zone, order) : NULL;

Code could be tidier and could avoid needless 80-column wordwrapping
and an unneeded local.

			page = NULL;
			if (__if_use_cma_first(zone, order, alloc_flags))
				page = __rmqueue_cma_fallback(zone, order);

>  			if (page)
>  				return page;
>  		}

Anyway, please take a look, fix the build error and send us a v3.  I
suggest you cc Minchan Kim, who might review it for us.
Roman Gushchin May 5, 2023, 10:28 p.m. UTC | #4 
On Thu, May 04, 2023 at 06:09:54PM +0800, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Let us look at the series of scenarios below with WMARK_LOW=25MB,WMARK_MIN=5MB
> (managed pages 1.9GB). We can know that current 'fixed 1/2 ratio' start to use
> CMA since C which actually has caused U&R lower than WMARK_LOW (this should be
> deemed as against current memory policy, that is, U&R should either stay around
> WATERMARK_LOW when no allocation or do reclaim via enter slowpath)
> 
> free_cma/free_pages(MB)      A(12/30)     B(12/25)     C(12/20)
> fixed 1/2 ratio                 N             N           Y
> this commit                     Y             Y           Y
> 
> Suggested-by: Roman Gushchin <roman.gushchin@linux.dev>

I didn't suggest it in this form, please, drop this tag.

> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
> v2: do proportion check when zone_watermark_ok, update commit message
> ---
> ---
>  mm/page_alloc.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0745aed..d0baeab 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3071,6 +3071,34 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>  
>  }
>  
> +#ifdef CONFIG_CMA
> +static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)
> +{
> +	unsigned long cma_proportion = 0;
> +	unsigned long cma_free_proportion = 0;
> +	unsigned long watermark = 0;
> +	long count = 0;
> +	bool cma_first = false;
> +
> +	watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> +	/*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/
> +	if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
> +		/* WMARK_LOW failed lead to using cma first, this helps U&R stay
> +		 * around low when being drained by GFP_MOVABLE
> +		 */
> +		cma_first = true;

This part looks reasonable to me.

> +	else {
> +		/*check proportion when zone_watermark_ok*/
> +		count = atomic_long_read(&zone->managed_pages);
> +		cma_proportion = zone->cma_pages * 100 / count;
> +		cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
> +			/  zone_page_state(zone, NR_FREE_PAGES);
> +		cma_first = (cma_free_proportion >= cma_proportion * 2

Why *2? Please, explain.

> +				|| cma_free_proportion >= 50);

It will heavily boost the use of cma at early stages of uptime, when there is a lot of !cma
memory, making continuous (e.g. hugetlb) allocations fail more often. Not a good idea.

Thanks!
Zhaoyang Huang May 6, 2023, 2:44 a.m. UTC | #5 
On Sat, May 6, 2023 at 6:29 AM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>
> On Thu, May 04, 2023 at 06:09:54PM +0800, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Let us look at the series of scenarios below with WMARK_LOW=25MB,WMARK_MIN=5MB
> > (managed pages 1.9GB). We can know that current 'fixed 1/2 ratio' start to use
> > CMA since C which actually has caused U&R lower than WMARK_LOW (this should be
> > deemed as against current memory policy, that is, U&R should either stay around
> > WATERMARK_LOW when no allocation or do reclaim via enter slowpath)
> >
> > free_cma/free_pages(MB)      A(12/30)     B(12/25)     C(12/20)
> > fixed 1/2 ratio                 N             N           Y
> > this commit                     Y             Y           Y
> >
> > Suggested-by: Roman Gushchin <roman.gushchin@linux.dev>
>
> I didn't suggest it in this form, please, drop this tag.
>
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> > v2: do proportion check when zone_watermark_ok, update commit message
> > ---
> > ---
> >  mm/page_alloc.c | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 0745aed..d0baeab 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3071,6 +3071,34 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
> >
> >  }
> >
> > +#ifdef CONFIG_CMA
> > +static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)
> > +{
> > +     unsigned long cma_proportion = 0;
> > +     unsigned long cma_free_proportion = 0;
> > +     unsigned long watermark = 0;
> > +     long count = 0;
> > +     bool cma_first = false;
> > +
> > +     watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> > +     /*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/
> > +     if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
> > +             /* WMARK_LOW failed lead to using cma first, this helps U&R stay
> > +              * around low when being drained by GFP_MOVABLE
> > +              */
> > +             cma_first = true;
>
> This part looks reasonable to me.
>
> > +     else {
> > +             /*check proportion when zone_watermark_ok*/
> > +             count = atomic_long_read(&zone->managed_pages);
> > +             cma_proportion = zone->cma_pages * 100 / count;
> > +             cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
> > +                     /  zone_page_state(zone, NR_FREE_PAGES);
> > +             cma_first = (cma_free_proportion >= cma_proportion * 2
>
> Why *2? Please, explain.
It is a magic number here which aims at avoiding late use of cma when
free pages near to WMARK_LOW by periodically using them in advance.
>
> > +                             || cma_free_proportion >= 50);
>
> It will heavily boost the use of cma at early stages of uptime, when there is a lot of !cma
> memory, making continuous (e.g. hugetlb) allocations fail more often. Not a good idea.
Actually, it is equal to "zone_page_state(zone, NR_FREE_CMA_PAGES) >
zone_page_state(zone, NR_FREE_PAGES) / 2"
>
> Thanks!
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0745aed..d0baeab 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3071,6 +3071,34 @@  static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
 
 }
 
+#ifdef CONFIG_CMA
+static bool __if_use_cma_first(struct zone *zone, unsigned int order, unsigned int alloc_flags)
+{
+	unsigned long cma_proportion = 0;
+	unsigned long cma_free_proportion = 0;
+	unsigned long watermark = 0;
+	long count = 0;
+	bool cma_first = false;
+
+	watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
+	/*check if GFP_MOVABLE pass previous watermark check via the help of CMA*/
+	if (!zone_watermark_ok(zone, order, watermark, 0, alloc_flags & (~ALLOC_CMA)))
+		/* WMARK_LOW failed lead to using cma first, this helps U&R stay
+		 * around low when being drained by GFP_MOVABLE
+		 */
+		cma_first = true;
+	else {
+		/*check proportion when zone_watermark_ok*/
+		count = atomic_long_read(&zone->managed_pages);
+		cma_proportion = zone->cma_pages * 100 / count;
+		cma_free_proportion = zone_page_state(zone, NR_FREE_CMA_PAGES) * 100
+			/  zone_page_state(zone, NR_FREE_PAGES);
+		cma_first = (cma_free_proportion >= cma_proportion * 2
+				|| cma_free_proportion >= 50);
+	}
+	return cma_first;
+}
+#endif
 /*
  * Do the hard work of removing an element from the buddy allocator.
  * Call me with the zone->lock already held.
@@ -3087,10 +3115,10 @@  static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
 		 * allocating from CMA when over half of the zone's free memory
 		 * is in the CMA area.
 		 */
-		if (alloc_flags & ALLOC_CMA &&
-		    zone_page_state(zone, NR_FREE_CMA_PAGES) >
-		    zone_page_state(zone, NR_FREE_PAGES) / 2) {
-			page = __rmqueue_cma_fallback(zone, order);
+		if (migratetype == MIGRATE_MOVABLE) {
+			bool cma_first = __if_use_cma_first(zone, order, alloc_flags);
+
+			page = cma_first ? __rmqueue_cma_fallback(zone, order) : NULL;
 			if (page)
 				return page;
 		}