From: Zhaoyang Huang <[email protected]>
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 <[email protected]>
Signed-off-by: Zhaoyang Huang <[email protected]>
---
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
add more reviewer
On Thu, May 4, 2023 at 6:11 PM zhaoyang.huang <[email protected]> wrote:
>
> From: Zhaoyang Huang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhaoyang Huang <[email protected]>
> ---
> 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
>
On Thu, 4 May 2023 18:09:54 +0800 "zhaoyang.huang" <[email protected]> wrote:
> From: Zhaoyang Huang <[email protected]>
>
> 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.
On Thu, May 04, 2023 at 06:09:54PM +0800, zhaoyang.huang wrote:
> From: Zhaoyang Huang <[email protected]>
>
> 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 <[email protected]>
I didn't suggest it in this form, please, drop this tag.
> Signed-off-by: Zhaoyang Huang <[email protected]>
> ---
> 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!
On Sat, May 6, 2023 at 6:29 AM Roman Gushchin <[email protected]> wrote:
>
> On Thu, May 04, 2023 at 06:09:54PM +0800, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <[email protected]>
> >
> > 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 <[email protected]>
>
> I didn't suggest it in this form, please, drop this tag.
>
> > Signed-off-by: Zhaoyang Huang <[email protected]>
> > ---
> > 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!