| Message ID | 1683194994-3070-1-git-send-email-zhaoyang.huang@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] mm: optimization on page allocation when CMA enabled | expand |
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
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 >
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.
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!
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 --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; }