[PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask

[PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask

[Uladzislau Rezki (Sony)]

That extra variable has been introduced just for keeping an original
passed gfp_mask because it is updated with __GFP_NOWARN on entry, thus
error handling messages were broken.

Instead we can keep an original gfp_mask without modifying it and add
an extra __GFP_NOWARN flag together with gfp_mask as a parameter to
the vm_area_alloc_pages() function. It will make it less confused.

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

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d2a00ad4e1dd..3b549ff5c95e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2920,7 +2920,6 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 				 int node)
 {
 	const gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;
-	const gfp_t orig_gfp_mask = gfp_mask;
 	unsigned long addr = (unsigned long)area->addr;
 	unsigned long size = get_vm_area_size(area);
 	unsigned long array_size;
@@ -2928,7 +2927,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	unsigned int page_order;
 
 	array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
-	gfp_mask |= __GFP_NOWARN;
+
 	if (!(gfp_mask & (GFP_DMA | GFP_DMA32)))
 		gfp_mask |= __GFP_HIGHMEM;
 
@@ -2941,7 +2940,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	}
 
 	if (!area->pages) {
-		warn_alloc(orig_gfp_mask, NULL,
+		warn_alloc(gfp_mask, NULL,
 			"vmalloc error: size %lu, failed to allocated page array size %lu",
 			nr_small_pages * PAGE_SIZE, array_size);
 		free_vm_area(area);
@@ -2951,8 +2950,8 @@ 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, node,
-		page_order, nr_small_pages, area->pages);
+	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
+		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
 
@@ -2961,7 +2960,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	 * allocation request, free them via __vfree() if any.
 	 */
 	if (area->nr_pages != nr_small_pages) {
-		warn_alloc(orig_gfp_mask, NULL,
+		warn_alloc(gfp_mask, NULL,
 			"vmalloc error: size %lu, page order %u, failed to allocate pages",
 			area->nr_pages * PAGE_SIZE, page_order);
 		goto fail;
@@ -2969,7 +2968,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 
 	if (vmap_pages_range(addr, addr + size, prot, area->pages,
 			page_shift) < 0) {
-		warn_alloc(orig_gfp_mask, NULL,
+		warn_alloc(gfp_mask, NULL,
 			"vmalloc error: size %lu, failed to map pages",
 			area->nr_pages * PAGE_SIZE);
 		goto fail;
-- 
2.17.1

Re: [PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask

[Michal Hocko]

[Cc Vasily]

On Wed 03-11-21 21:07:03, Uladzislau Rezki wrote:
> That extra variable has been introduced just for keeping an original
> passed gfp_mask because it is updated with __GFP_NOWARN on entry, thus
> error handling messages were broken.

I am not sure what you mean by "error handling messages were broken"
part.

It is true that the current Linus tree has a broken allocation failure
reporting but that is not a fault of orig_gfp_mask. In fact patch which
is fixing that "mm/vmalloc: repair warn_alloc()s in
__vmalloc_area_node()" currently in akpm tree is adding the additional
mask.
 
> Instead we can keep an original gfp_mask without modifying it and add
> an extra __GFP_NOWARN flag together with gfp_mask as a parameter to
> the vm_area_alloc_pages() function. It will make it less confused.

I would tend to agree that this is a better approach. There is already
nested_gfp mask and one more doesn't add to the readbility.

Maybe we should just drop the above patch and just go with one which
doesn't introduce the intermediate step and an additional gfp mask.

> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  mm/vmalloc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d2a00ad4e1dd..3b549ff5c95e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2920,7 +2920,6 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  				 int node)
>  {
>  	const gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;
> -	const gfp_t orig_gfp_mask = gfp_mask;
>  	unsigned long addr = (unsigned long)area->addr;
>  	unsigned long size = get_vm_area_size(area);
>  	unsigned long array_size;
> @@ -2928,7 +2927,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	unsigned int page_order;
>  
>  	array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
> -	gfp_mask |= __GFP_NOWARN;
> +
>  	if (!(gfp_mask & (GFP_DMA | GFP_DMA32)))
>  		gfp_mask |= __GFP_HIGHMEM;
>  
> @@ -2941,7 +2940,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	}
>  
>  	if (!area->pages) {
> -		warn_alloc(orig_gfp_mask, NULL,
> +		warn_alloc(gfp_mask, NULL,
>  			"vmalloc error: size %lu, failed to allocated page array size %lu",
>  			nr_small_pages * PAGE_SIZE, array_size);
>  		free_vm_area(area);
> @@ -2951,8 +2950,8 @@ 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, node,
> -		page_order, nr_small_pages, area->pages);
> +	area->nr_pages = vm_area_alloc_pages(gfp_mask | __GFP_NOWARN,
> +		node, page_order, nr_small_pages, area->pages);
>  
>  	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
>  
> @@ -2961,7 +2960,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	 * allocation request, free them via __vfree() if any.
>  	 */
>  	if (area->nr_pages != nr_small_pages) {
> -		warn_alloc(orig_gfp_mask, NULL,
> +		warn_alloc(gfp_mask, NULL,
>  			"vmalloc error: size %lu, page order %u, failed to allocate pages",
>  			area->nr_pages * PAGE_SIZE, page_order);
>  		goto fail;
> @@ -2969,7 +2968,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  
>  	if (vmap_pages_range(addr, addr + size, prot, area->pages,
>  			page_shift) < 0) {
> -		warn_alloc(orig_gfp_mask, NULL,
> +		warn_alloc(gfp_mask, NULL,
>  			"vmalloc error: size %lu, failed to map pages",
>  			area->nr_pages * PAGE_SIZE);
>  		goto fail;
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask

[Uladzislau Rezki]

> [Cc Vasily]
> 
> On Wed 03-11-21 21:07:03, Uladzislau Rezki wrote:
> > That extra variable has been introduced just for keeping an original
> > passed gfp_mask because it is updated with __GFP_NOWARN on entry, thus
> > error handling messages were broken.
> 
> I am not sure what you mean by "error handling messages were broken"
> part.
> 
We slightly discussed it in another thread :) There was __GFP_NOWARN added
on entry(unconditionally), what leads to ignoring all our internal error
messages by the warn_alloc(). I have checked the linux-next and saw that
Vasily sent a patch fixing it:

<snip>
Author: Vasily Averin <vvs@virtuozzo.com>
Date:   Thu Oct 21 15:07:26 2021 +1100

    mm/vmalloc: repair warn_alloc()s in __vmalloc_area_node()
    
    Commit f255935b9767 ("mm: cleanup the gfp_mask handling in
    __vmalloc_area_node") added __GFP_NOWARN to gfp_mask unconditionally
    however it disabled all output inside warn_alloc() call.  This patch saves
    original gfp_mask and provides it to all warn_alloc() calls.
<snip>

> It is true that the current Linus tree has a broken allocation failure
> reporting but that is not a fault of orig_gfp_mask. In fact patch which
> is fixing that "mm/vmalloc: repair warn_alloc()s in
> __vmalloc_area_node()" currently in akpm tree is adding the additional
> mask.
>  
> > Instead we can keep an original gfp_mask without modifying it and add
> > an extra __GFP_NOWARN flag together with gfp_mask as a parameter to
> > the vm_area_alloc_pages() function. It will make it less confused.
> 
> I would tend to agree that this is a better approach. There is already
> nested_gfp mask and one more doesn't add to the readbility.
> 
Agree, that is why i decided to send a patch. Because i find that extra
gfp variable as odd one and confusing. I paid an attention on it during
our discussion about __GFP_NOFAIL. But on my tree it was not fixed at all
and after checking the linux-next i saw a fix.

>
> Maybe we should just drop the above patch and just go with one which
> doesn't introduce the intermediate step and an additional gfp mask.
> 
That we can do if all agree on. 

Thanks!

--
Vlad Rezki