[PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Uladzislau Rezki (Sony)]

The aim is to simplify and making the vm_area_alloc_pages()
function less confusing as it became more clogged nowadays:

- eliminate a "bulk_gfp" variable and do not overwrite a gfp
  flag for bulk allocator;
- drop __GFP_NOFAIL flag for high-order-page requests on upper
  layer. It becomes less spread between levels when it comes to
  __GFP_NOFAIL allocations;
- add a comment about a fallback path if high-order attempt is
  unsuccessful because for such cases __GFP_NOFAIL is dropped;
- fix a typo in a commit message.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 3f9b6bd707d2..57862865e808 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
 		unsigned int order, unsigned int nr_pages, struct page **pages)
 {
 	unsigned int nr_allocated = 0;
-	gfp_t alloc_gfp = gfp;
-	bool nofail = gfp & __GFP_NOFAIL;
 	struct page *page;
 	int i;
 
@@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
 	 * more permissive.
 	 */
 	if (!order) {
-		/* bulk allocator doesn't support nofail req. officially */
-		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
-
 		while (nr_allocated < nr_pages) {
 			unsigned int nr, nr_pages_request;
 
@@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
 			 * but mempolicy wants to alloc memory by interleaving.
 			 */
 			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
-				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
+				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
 							nr_pages_request,
 							pages + nr_allocated);
-
 			else
-				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
+				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
 							nr_pages_request,
 							pages + nr_allocated);
 
@@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
 			if (nr != nr_pages_request)
 				break;
 		}
-	} else if (gfp & __GFP_NOFAIL) {
-		/*
-		 * Higher order nofail allocations are really expensive and
-		 * potentially dangerous (pre-mature OOM, disruptive reclaim
-		 * and compaction etc.
-		 */
-		alloc_gfp &= ~__GFP_NOFAIL;
 	}
 
 	/* High-order pages or fallback path if "bulk" fails. */
 	while (nr_allocated < nr_pages) {
-		if (!nofail && fatal_signal_pending(current))
+		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
 			break;
 
 		if (nid == NUMA_NO_NODE)
-			page = alloc_pages_noprof(alloc_gfp, order);
+			page = alloc_pages_noprof(gfp, order);
 		else
-			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
+			page = alloc_pages_node_noprof(nid, gfp, order);
+
 		if (unlikely(!page))
 			break;
 
 		/*
 		 * Higher order allocations must be able to be treated as
-		 * indepdenent small pages by callers (as they can with
+		 * independent small pages by callers (as they can with
 		 * small-page vmallocs). Some drivers do their own refcounting
 		 * on vmalloc_to_page() pages, some use page->mapping,
 		 * page->lru, etc.
@@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
 	page_order = vm_area_page_order(area);
 
-	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
+	/*
+	 * Higher order nofail allocations are really expensive and
+	 * potentially dangerous (pre-mature OOM, disruptive reclaim
+	 * and compaction etc.
+	 *
+	 * Please note, the __vmalloc_node_range_noprof() falls-back
+	 * to order-0 pages if high-order attempt is unsuccessful.
+	 */
+	area->nr_pages = vm_area_alloc_pages((page_order ?
+		gfp_mask & ~__GFP_NOFAIL : gfp_mask) | __GFP_NOWARN,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
-- 
2.39.2

Re: [PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Michal Hocko]

On Tue 27-08-24 21:09:16, Uladzislau Rezki wrote:
> The aim is to simplify and making the vm_area_alloc_pages()
> function less confusing as it became more clogged nowadays:
> 
> - eliminate a "bulk_gfp" variable and do not overwrite a gfp
>   flag for bulk allocator;
> - drop __GFP_NOFAIL flag for high-order-page requests on upper
>   layer. It becomes less spread between levels when it comes to
>   __GFP_NOFAIL allocations;
> - add a comment about a fallback path if high-order attempt is
>   unsuccessful because for such cases __GFP_NOFAIL is dropped;
> - fix a typo in a commit message.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/vmalloc.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3f9b6bd707d2..57862865e808 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  		unsigned int order, unsigned int nr_pages, struct page **pages)
>  {
>  	unsigned int nr_allocated = 0;
> -	gfp_t alloc_gfp = gfp;
> -	bool nofail = gfp & __GFP_NOFAIL;
>  	struct page *page;
>  	int i;
>  
> @@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  	 * more permissive.
>  	 */
>  	if (!order) {
> -		/* bulk allocator doesn't support nofail req. officially */
> -		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
> -
>  		while (nr_allocated < nr_pages) {
>  			unsigned int nr, nr_pages_request;
>  
> @@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  			 * but mempolicy wants to alloc memory by interleaving.
>  			 */
>  			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
> -				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
> +				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
>  							nr_pages_request,
>  							pages + nr_allocated);
> -
>  			else
> -				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
> +				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
>  							nr_pages_request,
>  							pages + nr_allocated);
>  
> @@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  			if (nr != nr_pages_request)
>  				break;
>  		}
> -	} else if (gfp & __GFP_NOFAIL) {
> -		/*
> -		 * Higher order nofail allocations are really expensive and
> -		 * potentially dangerous (pre-mature OOM, disruptive reclaim
> -		 * and compaction etc.
> -		 */
> -		alloc_gfp &= ~__GFP_NOFAIL;
>  	}
>  
>  	/* High-order pages or fallback path if "bulk" fails. */
>  	while (nr_allocated < nr_pages) {
> -		if (!nofail && fatal_signal_pending(current))
> +		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
>  			break;
>  
>  		if (nid == NUMA_NO_NODE)
> -			page = alloc_pages_noprof(alloc_gfp, order);
> +			page = alloc_pages_noprof(gfp, order);
>  		else
> -			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
> +			page = alloc_pages_node_noprof(nid, gfp, order);
> +
>  		if (unlikely(!page))
>  			break;
>  
>  		/*
>  		 * Higher order allocations must be able to be treated as
> -		 * indepdenent small pages by callers (as they can with
> +		 * independent small pages by callers (as they can with
>  		 * small-page vmallocs). Some drivers do their own refcounting
>  		 * on vmalloc_to_page() pages, some use page->mapping,
>  		 * page->lru, etc.
> @@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
>  	page_order = vm_area_page_order(area);
>  
> -	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> +	/*
> +	 * Higher order nofail allocations are really expensive and
> +	 * potentially dangerous (pre-mature OOM, disruptive reclaim
> +	 * and compaction etc.
> +	 *
> +	 * Please note, the __vmalloc_node_range_noprof() falls-back
> +	 * to order-0 pages if high-order attempt is unsuccessful.
> +	 */
> +	area->nr_pages = vm_area_alloc_pages((page_order ?
> +		gfp_mask & ~__GFP_NOFAIL : gfp_mask) | __GFP_NOWARN,
>  		node, page_order, nr_small_pages, area->pages);
>  
>  	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
> -- 
> 2.39.2

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Baoquan He]

On 08/27/24 at 09:09pm, Uladzislau Rezki (Sony) wrote:
> The aim is to simplify and making the vm_area_alloc_pages()
> function less confusing as it became more clogged nowadays:
> 
> - eliminate a "bulk_gfp" variable and do not overwrite a gfp
>   flag for bulk allocator;
> - drop __GFP_NOFAIL flag for high-order-page requests on upper
>   layer. It becomes less spread between levels when it comes to
>   __GFP_NOFAIL allocations;
> - add a comment about a fallback path if high-order attempt is
>   unsuccessful because for such cases __GFP_NOFAIL is dropped;
> - fix a typo in a commit message.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  mm/vmalloc.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3f9b6bd707d2..57862865e808 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  		unsigned int order, unsigned int nr_pages, struct page **pages)
>  {
>  	unsigned int nr_allocated = 0;
> -	gfp_t alloc_gfp = gfp;
> -	bool nofail = gfp & __GFP_NOFAIL;
>  	struct page *page;
>  	int i;
>  
> @@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  	 * more permissive.
>  	 */
>  	if (!order) {
> -		/* bulk allocator doesn't support nofail req. officially */
> -		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
> -
>  		while (nr_allocated < nr_pages) {
>  			unsigned int nr, nr_pages_request;
>  
> @@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  			 * but mempolicy wants to alloc memory by interleaving.
>  			 */
>  			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
> -				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
> +				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
>  							nr_pages_request,
>  							pages + nr_allocated);
> -
>  			else
> -				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
> +				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
>  							nr_pages_request,
>  							pages + nr_allocated);
>  
> @@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  			if (nr != nr_pages_request)
>  				break;
>  		}
> -	} else if (gfp & __GFP_NOFAIL) {
> -		/*
> -		 * Higher order nofail allocations are really expensive and
> -		 * potentially dangerous (pre-mature OOM, disruptive reclaim
> -		 * and compaction etc.
> -		 */
> -		alloc_gfp &= ~__GFP_NOFAIL;
>  	}
>  
>  	/* High-order pages or fallback path if "bulk" fails. */
>  	while (nr_allocated < nr_pages) {
> -		if (!nofail && fatal_signal_pending(current))
> +		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
>  			break;
>  
>  		if (nid == NUMA_NO_NODE)
> -			page = alloc_pages_noprof(alloc_gfp, order);
> +			page = alloc_pages_noprof(gfp, order);
>  		else
> -			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
> +			page = alloc_pages_node_noprof(nid, gfp, order);
> +
>  		if (unlikely(!page))
>  			break;
>  
>  		/*
>  		 * Higher order allocations must be able to be treated as
> -		 * indepdenent small pages by callers (as they can with
> +		 * independent small pages by callers (as they can with
>  		 * small-page vmallocs). Some drivers do their own refcounting
>  		 * on vmalloc_to_page() pages, some use page->mapping,
>  		 * page->lru, etc.
> @@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
>  	page_order = vm_area_page_order(area);
>  
> -	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> +	/*
> +	 * Higher order nofail allocations are really expensive and
           ~~~~~~~~~~~~
Seems we use both higher-order and high-order to describe the
non 0-order pages in many places. I personally would take high-order,
higher-order seems to be a little confusing because it's not explicit
what is compared with and lower.

Surely this is not an issue to this patch, I see a lot of 'higher order'
in kernel codes.

For this patch,

Reviewed-by: Baoquan He <bhe@redhat.com>

> +	 * potentially dangerous (pre-mature OOM, disruptive reclaim
> +	 * and compaction etc.
> +	 *
> +	 * Please note, the __vmalloc_node_range_noprof() falls-back
> +	 * to order-0 pages if high-order attempt is unsuccessful.
> +	 */
> +	area->nr_pages = vm_area_alloc_pages((page_order ?
> +		gfp_mask & ~__GFP_NOFAIL : gfp_mask) | __GFP_NOWARN,
>  		node, page_order, nr_small_pages, area->pages);
>  
>  	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
> -- 
> 2.39.2
> 

Re: [PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Uladzislau Rezki]

On Thu, Aug 29, 2024 at 11:48:32AM +0800, Baoquan He wrote:
> On 08/27/24 at 09:09pm, Uladzislau Rezki (Sony) wrote:
> > The aim is to simplify and making the vm_area_alloc_pages()
> > function less confusing as it became more clogged nowadays:
> > 
> > - eliminate a "bulk_gfp" variable and do not overwrite a gfp
> >   flag for bulk allocator;
> > - drop __GFP_NOFAIL flag for high-order-page requests on upper
> >   layer. It becomes less spread between levels when it comes to
> >   __GFP_NOFAIL allocations;
> > - add a comment about a fallback path if high-order attempt is
> >   unsuccessful because for such cases __GFP_NOFAIL is dropped;
> > - fix a typo in a commit message.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  mm/vmalloc.c | 37 +++++++++++++++++--------------------
> >  1 file changed, 17 insertions(+), 20 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 3f9b6bd707d2..57862865e808 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> >  		unsigned int order, unsigned int nr_pages, struct page **pages)
> >  {
> >  	unsigned int nr_allocated = 0;
> > -	gfp_t alloc_gfp = gfp;
> > -	bool nofail = gfp & __GFP_NOFAIL;
> >  	struct page *page;
> >  	int i;
> >  
> > @@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> >  	 * more permissive.
> >  	 */
> >  	if (!order) {
> > -		/* bulk allocator doesn't support nofail req. officially */
> > -		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
> > -
> >  		while (nr_allocated < nr_pages) {
> >  			unsigned int nr, nr_pages_request;
> >  
> > @@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> >  			 * but mempolicy wants to alloc memory by interleaving.
> >  			 */
> >  			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
> > -				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
> > +				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
> >  							nr_pages_request,
> >  							pages + nr_allocated);
> > -
> >  			else
> > -				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
> > +				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
> >  							nr_pages_request,
> >  							pages + nr_allocated);
> >  
> > @@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> >  			if (nr != nr_pages_request)
> >  				break;
> >  		}
> > -	} else if (gfp & __GFP_NOFAIL) {
> > -		/*
> > -		 * Higher order nofail allocations are really expensive and
> > -		 * potentially dangerous (pre-mature OOM, disruptive reclaim
> > -		 * and compaction etc.
> > -		 */
> > -		alloc_gfp &= ~__GFP_NOFAIL;
> >  	}
> >  
> >  	/* High-order pages or fallback path if "bulk" fails. */
> >  	while (nr_allocated < nr_pages) {
> > -		if (!nofail && fatal_signal_pending(current))
> > +		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
> >  			break;
> >  
> >  		if (nid == NUMA_NO_NODE)
> > -			page = alloc_pages_noprof(alloc_gfp, order);
> > +			page = alloc_pages_noprof(gfp, order);
> >  		else
> > -			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
> > +			page = alloc_pages_node_noprof(nid, gfp, order);
> > +
> >  		if (unlikely(!page))
> >  			break;
> >  
> >  		/*
> >  		 * Higher order allocations must be able to be treated as
> > -		 * indepdenent small pages by callers (as they can with
> > +		 * independent small pages by callers (as they can with
> >  		 * small-page vmallocs). Some drivers do their own refcounting
> >  		 * on vmalloc_to_page() pages, some use page->mapping,
> >  		 * page->lru, etc.
> > @@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >  	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
> >  	page_order = vm_area_page_order(area);
> >  
> > -	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> > +	/*
> > +	 * Higher order nofail allocations are really expensive and
>            ~~~~~~~~~~~~
> Seems we use both higher-order and high-order to describe the
> non 0-order pages in many places. I personally would take high-order,
> higher-order seems to be a little confusing because it's not explicit
> what is compared with and lower.
> 
> Surely this is not an issue to this patch, I see a lot of 'higher order'
> in kernel codes.
> 
I agree. It sounds like hard to figure out the difference between both.
Are you willing send the patch? If not, i can send it out :)

> For this patch,
> 
> Reviewed-by: Baoquan He <bhe@redhat.com>
> 
Thanks!

--
Uladzislau Rezki

Re: [PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Baoquan He]

On 08/29/24 at 10:12am, Uladzislau Rezki wrote:
> On Thu, Aug 29, 2024 at 11:48:32AM +0800, Baoquan He wrote:
> > On 08/27/24 at 09:09pm, Uladzislau Rezki (Sony) wrote:
> > > The aim is to simplify and making the vm_area_alloc_pages()
> > > function less confusing as it became more clogged nowadays:
> > > 
> > > - eliminate a "bulk_gfp" variable and do not overwrite a gfp
> > >   flag for bulk allocator;
> > > - drop __GFP_NOFAIL flag for high-order-page requests on upper
> > >   layer. It becomes less spread between levels when it comes to
> > >   __GFP_NOFAIL allocations;
> > > - add a comment about a fallback path if high-order attempt is
> > >   unsuccessful because for such cases __GFP_NOFAIL is dropped;
> > > - fix a typo in a commit message.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  mm/vmalloc.c | 37 +++++++++++++++++--------------------
> > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 3f9b6bd707d2..57862865e808 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > >  		unsigned int order, unsigned int nr_pages, struct page **pages)
> > >  {
> > >  	unsigned int nr_allocated = 0;
> > > -	gfp_t alloc_gfp = gfp;
> > > -	bool nofail = gfp & __GFP_NOFAIL;
> > >  	struct page *page;
> > >  	int i;
> > >  
> > > @@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > >  	 * more permissive.
> > >  	 */
> > >  	if (!order) {
> > > -		/* bulk allocator doesn't support nofail req. officially */
> > > -		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
> > > -
> > >  		while (nr_allocated < nr_pages) {
> > >  			unsigned int nr, nr_pages_request;
> > >  
> > > @@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > >  			 * but mempolicy wants to alloc memory by interleaving.
> > >  			 */
> > >  			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
> > > -				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
> > > +				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
> > >  							nr_pages_request,
> > >  							pages + nr_allocated);
> > > -
> > >  			else
> > > -				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
> > > +				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
> > >  							nr_pages_request,
> > >  							pages + nr_allocated);
> > >  
> > > @@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > >  			if (nr != nr_pages_request)
> > >  				break;
> > >  		}
> > > -	} else if (gfp & __GFP_NOFAIL) {
> > > -		/*
> > > -		 * Higher order nofail allocations are really expensive and
> > > -		 * potentially dangerous (pre-mature OOM, disruptive reclaim
> > > -		 * and compaction etc.
> > > -		 */
> > > -		alloc_gfp &= ~__GFP_NOFAIL;
> > >  	}
> > >  
> > >  	/* High-order pages or fallback path if "bulk" fails. */
> > >  	while (nr_allocated < nr_pages) {
> > > -		if (!nofail && fatal_signal_pending(current))
> > > +		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
> > >  			break;
> > >  
> > >  		if (nid == NUMA_NO_NODE)
> > > -			page = alloc_pages_noprof(alloc_gfp, order);
> > > +			page = alloc_pages_noprof(gfp, order);
> > >  		else
> > > -			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
> > > +			page = alloc_pages_node_noprof(nid, gfp, order);
> > > +
> > >  		if (unlikely(!page))
> > >  			break;
> > >  
> > >  		/*
> > >  		 * Higher order allocations must be able to be treated as
> > > -		 * indepdenent small pages by callers (as they can with
> > > +		 * independent small pages by callers (as they can with
> > >  		 * small-page vmallocs). Some drivers do their own refcounting
> > >  		 * on vmalloc_to_page() pages, some use page->mapping,
> > >  		 * page->lru, etc.
> > > @@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > >  	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
> > >  	page_order = vm_area_page_order(area);
> > >  
> > > -	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> > > +	/*
> > > +	 * Higher order nofail allocations are really expensive and
> >            ~~~~~~~~~~~~
> > Seems we use both higher-order and high-order to describe the
> > non 0-order pages in many places. I personally would take high-order,
> > higher-order seems to be a little confusing because it's not explicit
> > what is compared with and lower.
> > 
> > Surely this is not an issue to this patch, I see a lot of 'higher order'
> > in kernel codes.
> > 
> I agree. It sounds like hard to figure out the difference between both.
> Are you willing send the patch? If not, i can send it out :)

I am fine, please go ahead.

Re: [PATCH] mm: vmalloc: Refactor vm_area_alloc_pages() function

[Uladzislau Rezki]

On Thu, Aug 29, 2024 at 04:44:51PM +0800, Baoquan He wrote:
> On 08/29/24 at 10:12am, Uladzislau Rezki wrote:
> > On Thu, Aug 29, 2024 at 11:48:32AM +0800, Baoquan He wrote:
> > > On 08/27/24 at 09:09pm, Uladzislau Rezki (Sony) wrote:
> > > > The aim is to simplify and making the vm_area_alloc_pages()
> > > > function less confusing as it became more clogged nowadays:
> > > > 
> > > > - eliminate a "bulk_gfp" variable and do not overwrite a gfp
> > > >   flag for bulk allocator;
> > > > - drop __GFP_NOFAIL flag for high-order-page requests on upper
> > > >   layer. It becomes less spread between levels when it comes to
> > > >   __GFP_NOFAIL allocations;
> > > > - add a comment about a fallback path if high-order attempt is
> > > >   unsuccessful because for such cases __GFP_NOFAIL is dropped;
> > > > - fix a typo in a commit message.
> > > > 
> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > ---
> > > >  mm/vmalloc.c | 37 +++++++++++++++++--------------------
> > > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > index 3f9b6bd707d2..57862865e808 100644
> > > > --- a/mm/vmalloc.c
> > > > +++ b/mm/vmalloc.c
> > > > @@ -3531,8 +3531,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > > >  		unsigned int order, unsigned int nr_pages, struct page **pages)
> > > >  {
> > > >  	unsigned int nr_allocated = 0;
> > > > -	gfp_t alloc_gfp = gfp;
> > > > -	bool nofail = gfp & __GFP_NOFAIL;
> > > >  	struct page *page;
> > > >  	int i;
> > > >  
> > > > @@ -3543,9 +3541,6 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > > >  	 * more permissive.
> > > >  	 */
> > > >  	if (!order) {
> > > > -		/* bulk allocator doesn't support nofail req. officially */
> > > > -		gfp_t bulk_gfp = gfp & ~__GFP_NOFAIL;
> > > > -
> > > >  		while (nr_allocated < nr_pages) {
> > > >  			unsigned int nr, nr_pages_request;
> > > >  
> > > > @@ -3563,12 +3558,11 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > > >  			 * but mempolicy wants to alloc memory by interleaving.
> > > >  			 */
> > > >  			if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
> > > > -				nr = alloc_pages_bulk_array_mempolicy_noprof(bulk_gfp,
> > > > +				nr = alloc_pages_bulk_array_mempolicy_noprof(gfp,
> > > >  							nr_pages_request,
> > > >  							pages + nr_allocated);
> > > > -
> > > >  			else
> > > > -				nr = alloc_pages_bulk_array_node_noprof(bulk_gfp, nid,
> > > > +				nr = alloc_pages_bulk_array_node_noprof(gfp, nid,
> > > >  							nr_pages_request,
> > > >  							pages + nr_allocated);
> > > >  
> > > > @@ -3582,30 +3576,24 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> > > >  			if (nr != nr_pages_request)
> > > >  				break;
> > > >  		}
> > > > -	} else if (gfp & __GFP_NOFAIL) {
> > > > -		/*
> > > > -		 * Higher order nofail allocations are really expensive and
> > > > -		 * potentially dangerous (pre-mature OOM, disruptive reclaim
> > > > -		 * and compaction etc.
> > > > -		 */
> > > > -		alloc_gfp &= ~__GFP_NOFAIL;
> > > >  	}
> > > >  
> > > >  	/* High-order pages or fallback path if "bulk" fails. */
> > > >  	while (nr_allocated < nr_pages) {
> > > > -		if (!nofail && fatal_signal_pending(current))
> > > > +		if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
> > > >  			break;
> > > >  
> > > >  		if (nid == NUMA_NO_NODE)
> > > > -			page = alloc_pages_noprof(alloc_gfp, order);
> > > > +			page = alloc_pages_noprof(gfp, order);
> > > >  		else
> > > > -			page = alloc_pages_node_noprof(nid, alloc_gfp, order);
> > > > +			page = alloc_pages_node_noprof(nid, gfp, order);
> > > > +
> > > >  		if (unlikely(!page))
> > > >  			break;
> > > >  
> > > >  		/*
> > > >  		 * Higher order allocations must be able to be treated as
> > > > -		 * indepdenent small pages by callers (as they can with
> > > > +		 * independent small pages by callers (as they can with
> > > >  		 * small-page vmallocs). Some drivers do their own refcounting
> > > >  		 * on vmalloc_to_page() pages, some use page->mapping,
> > > >  		 * page->lru, etc.
> > > > @@ -3666,7 +3654,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > >  	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
> > > >  	page_order = vm_area_page_order(area);
> > > >  
> > > > -	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> > > > +	/*
> > > > +	 * Higher order nofail allocations are really expensive and
> > >            ~~~~~~~~~~~~
> > > Seems we use both higher-order and high-order to describe the
> > > non 0-order pages in many places. I personally would take high-order,
> > > higher-order seems to be a little confusing because it's not explicit
> > > what is compared with and lower.
> > > 
> > > Surely this is not an issue to this patch, I see a lot of 'higher order'
> > > in kernel codes.
> > > 
> > I agree. It sounds like hard to figure out the difference between both.
> > Are you willing send the patch? If not, i can send it out :)
> 
> I am fine, please go ahead.
> 
Good! I will fix it.

--
Uladzislau Rezki