linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mm/vmalloc: Switch to bulk allocator in __vmalloc_area_node()
@ 2021-05-09 19:38 Uladzislau Rezki (Sony)
  2021-05-09 19:38 ` [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure Uladzislau Rezki (Sony)
  0 siblings, 1 reply; 11+ messages in thread
From: Uladzislau Rezki (Sony) @ 2021-05-09 19:38 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, LKML, Matthew Wilcox, Nicholas Piggin, Mel Gorman,
	Uladzislau Rezki, Hillf Danton, Michal Hocko,
	Oleksiy Avramchenko, Steven Rostedt

Recently there has been introduced a page bulk allocator for
users which need to get number of pages per one call request.

For order-0 pages switch to __alloc_pages_bulk() instead of
alloc_pages_node(), the reason is the former is not capable
of allocating set of pages, thus a one call is per one page.

Second, according to my tests the bulk allocator uses less
cycles even for scenarios when only one page is requested.
Running the "perf" on same test case shows below difference:

<default>
  - 45.18% __vmalloc_node
     - __vmalloc_node_range
        - 35.60% __alloc_pages
           - get_page_from_freelist
                3.36% __list_del_entry_valid
                3.00% check_preemption_disabled
                1.42% prep_new_page
<default>

<patch>
  - 31.00% __vmalloc_node
     - __vmalloc_node_range
        - 14.48% __alloc_pages_bulk
             3.22% __list_del_entry_valid
           - 0.83% __alloc_pages
                get_page_from_freelist
<patch>

The "test_vmalloc.sh" also shows performance improvements:

fix_size_alloc_test_4MB   loops: 1000000 avg: 89105095 usec
fix_size_alloc_test       loops: 1000000 avg: 513672   usec
full_fit_alloc_test       loops: 1000000 avg: 748900   usec
long_busy_list_alloc_test loops: 1000000 avg: 8043038  usec
random_size_alloc_test    loops: 1000000 avg: 4028582  usec
fix_align_alloc_test      loops: 1000000 avg: 1457671  usec

fix_size_alloc_test_4MB   loops: 1000000 avg: 62083711 usec
fix_size_alloc_test       loops: 1000000 avg: 449207   usec
full_fit_alloc_test       loops: 1000000 avg: 735985   usec
long_busy_list_alloc_test loops: 1000000 avg: 5176052  usec
random_size_alloc_test    loops: 1000000 avg: 2589252  usec
fix_align_alloc_test      loops: 1000000 avg: 1365009  usec

For example 4MB allocations illustrates ~30% gain, all the
rest is also better.

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

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 5d96fee17226..dbc6744400d5 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2766,8 +2766,6 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	unsigned long array_size;
 	unsigned int nr_small_pages = size >> PAGE_SHIFT;
 	unsigned int page_order;
-	struct page **pages;
-	unsigned int i;
 
 	array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
 	gfp_mask |= __GFP_NOWARN;
@@ -2776,13 +2774,13 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 
 	/* Please note that the recursion is strictly bounded. */
 	if (array_size > PAGE_SIZE) {
-		pages = __vmalloc_node(array_size, 1, nested_gfp, node,
+		area->pages = __vmalloc_node(array_size, 1, nested_gfp, node,
 					area->caller);
 	} else {
-		pages = kmalloc_node(array_size, nested_gfp, node);
+		area->pages = kmalloc_node(array_size, nested_gfp, node);
 	}
 
-	if (!pages) {
+	if (!area->pages) {
 		free_vm_area(area);
 		warn_alloc(gfp_mask, NULL,
 			   "vmalloc size %lu allocation failure: "
@@ -2791,43 +2789,51 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		return NULL;
 	}
 
-	area->pages = pages;
-	area->nr_pages = nr_small_pages;
 	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
-
 	page_order = vm_area_page_order(area);
 
-	/*
-	 * Careful, we allocate and map page_order pages, but tracking is done
-	 * per PAGE_SIZE page so as to keep the vm_struct APIs independent of
-	 * the physical/mapped size.
-	 */
-	for (i = 0; i < area->nr_pages; i += 1U << page_order) {
-		struct page *page;
-		int p;
-
-		/* Compound pages required for remap_vmalloc_page */
-		page = alloc_pages_node(node, gfp_mask | __GFP_COMP, page_order);
-		if (unlikely(!page)) {
-			/* Successfully allocated i pages, free them in __vfree() */
-			area->nr_pages = i;
-			atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
-			warn_alloc(gfp_mask, NULL,
-				   "vmalloc size %lu allocation failure: "
-				   "page order %u allocation failed",
-				   area->nr_pages * PAGE_SIZE, page_order);
-			goto fail;
-		}
+	if (!page_order) {
+		area->nr_pages = __alloc_pages_bulk(gfp_mask, node,
+			NULL, nr_small_pages, NULL, area->pages);
+	} else {
+		/*
+		 * Careful, we allocate and map page_order pages, but tracking is done
+		 * per PAGE_SIZE page so as to keep the vm_struct APIs independent of
+		 * the physical/mapped size.
+		 */
+		for (area->nr_pages = 0; area->nr_pages < nr_small_pages;
+				area->nr_pages += 1U << page_order) {
+			struct page *page;
+			int i;
+
+			/* Compound pages required for remap_vmalloc_page */
+			page = alloc_pages_node(node, gfp_mask | __GFP_COMP, page_order);
+			if (unlikely(!page))
+				break;
 
-		for (p = 0; p < (1U << page_order); p++)
-			area->pages[i + p] = page + p;
+			for (i = 0; i < (1U << page_order); i++)
+				area->pages[area->nr_pages + i] = page + i;
 
-		if (gfpflags_allow_blocking(gfp_mask))
-			cond_resched();
+			if (gfpflags_allow_blocking(gfp_mask))
+				cond_resched();
+		}
 	}
+
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
 
-	if (vmap_pages_range(addr, addr + size, prot, pages, page_shift) < 0) {
+	/*
+	 * If not enough pages were obtained to accomplish an
+	 * allocation request, free them via __vfree() if any.
+	 */
+	if (area->nr_pages != nr_small_pages) {
+		warn_alloc(gfp_mask, NULL,
+			"vmalloc size %lu allocation failure: "
+			"page order %u allocation failed",
+			area->nr_pages * PAGE_SIZE, page_order);
+		goto fail;
+	}
+
+	if (vmap_pages_range(addr, addr + size, prot, area->pages, page_shift) < 0) {
 		warn_alloc(gfp_mask, NULL,
 			   "vmalloc size %lu allocation failure: "
 			   "failed to map pages",
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 19:38 [PATCH v2 1/2] mm/vmalloc: Switch to bulk allocator in __vmalloc_area_node() Uladzislau Rezki (Sony)
@ 2021-05-09 19:38 ` Uladzislau Rezki (Sony)
  2021-05-09 19:46   ` Uladzislau Rezki
  2021-05-09 19:47   ` Matthew Wilcox
  0 siblings, 2 replies; 11+ messages in thread
From: Uladzislau Rezki (Sony) @ 2021-05-09 19:38 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, LKML, Matthew Wilcox, Nicholas Piggin, Mel Gorman,
	Uladzislau Rezki, Hillf Danton, Michal Hocko,
	Oleksiy Avramchenko, Steven Rostedt

When a memory allocation for array of pages are not succeed
emit a warning message as a first step and then perform the
further cleanup.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 Makefile     | 2 +-
 mm/vmalloc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index bb50a5ac2e13..1d658e171495 100644
--- a/Makefile
+++ b/Makefile
@@ -430,7 +430,7 @@ HOSTCXX	= g++
 endif
 
 export KBUILD_USERCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \
-			      -O2 -fomit-frame-pointer -std=gnu89
+			      -O0 -g -fomit-frame-pointer -std=gnu89
 export KBUILD_USERLDFLAGS :=
 
 KBUILD_HOSTCFLAGS   := $(KBUILD_USERCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index dbc6744400d5..1f664a17d9ea 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	}
 
 	if (!area->pages) {
-		free_vm_area(area);
 		warn_alloc(gfp_mask, NULL,
 			   "vmalloc size %lu allocation failure: "
 			   "page array size %lu allocation failed",
 			   nr_small_pages * PAGE_SIZE, array_size);
+		free_vm_area(area);
 		return NULL;
 	}
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 19:38 ` [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure Uladzislau Rezki (Sony)
@ 2021-05-09 19:46   ` Uladzislau Rezki
  2021-05-09 19:47   ` Matthew Wilcox
  1 sibling, 0 replies; 11+ messages in thread
From: Uladzislau Rezki @ 2021-05-09 19:46 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Andrew Morton, linux-mm, LKML, Matthew Wilcox, Nicholas Piggin,
	Mel Gorman, Hillf Danton, Michal Hocko, Oleksiy Avramchenko,
	Steven Rostedt

> When a memory allocation for array of pages are not succeed
> emit a warning message as a first step and then perform the
> further cleanup.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  Makefile     | 2 +-
>  mm/vmalloc.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index bb50a5ac2e13..1d658e171495 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -430,7 +430,7 @@ HOSTCXX	= g++
>  endif
>  
>  export KBUILD_USERCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \
> -			      -O2 -fomit-frame-pointer -std=gnu89
> +			      -O0 -g -fomit-frame-pointer -std=gnu89
>  export KBUILD_USERLDFLAGS :=
>  
>  KBUILD_HOSTCFLAGS   := $(KBUILD_USERCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS)
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index dbc6744400d5..1f664a17d9ea 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	}
>  
>  	if (!area->pages) {
> -		free_vm_area(area);
>  		warn_alloc(gfp_mask, NULL,
>  			   "vmalloc size %lu allocation failure: "
>  			   "page array size %lu allocation failed",
>  			   nr_small_pages * PAGE_SIZE, array_size);
> +		free_vm_area(area);
>  		return NULL;
>  	}
>  
> -- 
> 2.20.1
> 
Please consider a V3 of this patch. Apparently a modification of
Makefile was included in the patch. What is wrong :)

From 9c484d83a630c4430dcb055a26c879d8e3c5cae1 Mon Sep 17 00:00:00 2001
From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Date: Sat, 8 May 2021 23:41:21 +0200
Subject: [PATCH v3 2/2] mm/vmalloc: Print a warning message first on failure

When a memory allocation for array of pages are not succeed
emit a warning message as a first step and then perform the
further cleanup.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index dbc6744400d5..1f664a17d9ea 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	}
 
 	if (!area->pages) {
-		free_vm_area(area);
 		warn_alloc(gfp_mask, NULL,
 			   "vmalloc size %lu allocation failure: "
 			   "page array size %lu allocation failed",
 			   nr_small_pages * PAGE_SIZE, array_size);
+		free_vm_area(area);
 		return NULL;
 	}
 
-- 
2.20.1

--
Vlad Rezki

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 19:38 ` [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure Uladzislau Rezki (Sony)
  2021-05-09 19:46   ` Uladzislau Rezki
@ 2021-05-09 19:47   ` Matthew Wilcox
  2021-05-09 20:05     ` Uladzislau Rezki
  1 sibling, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2021-05-09 19:47 UTC (permalink / raw)
  To: Uladzislau Rezki (Sony)
  Cc: Andrew Morton, linux-mm, LKML, Nicholas Piggin, Mel Gorman,
	Hillf Danton, Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Sun, May 09, 2021 at 09:38:44PM +0200, Uladzislau Rezki (Sony) wrote:
>  export KBUILD_USERCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \
> -			      -O2 -fomit-frame-pointer -std=gnu89
> +			      -O0 -g -fomit-frame-pointer -std=gnu89

You clearly didn't intend to submit this portion ...

> +++ b/mm/vmalloc.c
> @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	}
>  
>  	if (!area->pages) {
> -		free_vm_area(area);
>  		warn_alloc(gfp_mask, NULL,
>  			   "vmalloc size %lu allocation failure: "
>  			   "page array size %lu allocation failed",
>  			   nr_small_pages * PAGE_SIZE, array_size);
> +		free_vm_area(area);
>  		return NULL;
>  	}

I think this is a bad idea.  We're clearly low on memory (a memory
allocation just failed).  We should free the memory we have allocated
to improve the chances of the warning message making it out.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 19:47   ` Matthew Wilcox
@ 2021-05-09 20:05     ` Uladzislau Rezki
  2021-05-09 20:18       ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Uladzislau Rezki @ 2021-05-09 20:05 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Uladzislau Rezki (Sony),
	Andrew Morton, linux-mm, LKML, Nicholas Piggin, Mel Gorman,
	Hillf Danton, Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Sun, May 09, 2021 at 08:47:12PM +0100, Matthew Wilcox wrote:
> On Sun, May 09, 2021 at 09:38:44PM +0200, Uladzislau Rezki (Sony) wrote:
> >  export KBUILD_USERCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \
> > -			      -O2 -fomit-frame-pointer -std=gnu89
> > +			      -O0 -g -fomit-frame-pointer -std=gnu89
> 
> You clearly didn't intend to submit this portion ...
> 
I sent a v3 that does not include it. That was added accidentally.

> > +++ b/mm/vmalloc.c
> > @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >  	}
> >  
> >  	if (!area->pages) {
> > -		free_vm_area(area);
> >  		warn_alloc(gfp_mask, NULL,
> >  			   "vmalloc size %lu allocation failure: "
> >  			   "page array size %lu allocation failed",
> >  			   nr_small_pages * PAGE_SIZE, array_size);
> > +		free_vm_area(area);
> >  		return NULL;
> >  	}
> 
> I think this is a bad idea.  We're clearly low on memory (a memory
> allocation just failed).  We should free the memory we have allocated
> to improve the chances of the warning message making it out.
Not sure if i fully follow you here. We do free the memory. The intention
was to print a warning message first because, if, potentially, the
free_vm_area(area) also does some prints it would be a bit messy from the
point what has been broken first.

So, could you please clarify what was your concern?

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 20:05     ` Uladzislau Rezki
@ 2021-05-09 20:18       ` Matthew Wilcox
  2021-05-09 21:26         ` Uladzislau Rezki
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2021-05-09 20:18 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Andrew Morton, linux-mm, LKML, Nicholas Piggin, Mel Gorman,
	Hillf Danton, Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Sun, May 09, 2021 at 10:05:19PM +0200, Uladzislau Rezki wrote:
> On Sun, May 09, 2021 at 08:47:12PM +0100, Matthew Wilcox wrote:
> > > +++ b/mm/vmalloc.c
> > > @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > >  	}
> > >  
> > >  	if (!area->pages) {
> > > -		free_vm_area(area);
> > >  		warn_alloc(gfp_mask, NULL,
> > >  			   "vmalloc size %lu allocation failure: "
> > >  			   "page array size %lu allocation failed",
> > >  			   nr_small_pages * PAGE_SIZE, array_size);
> > > +		free_vm_area(area);
> > >  		return NULL;
> > >  	}
> > 
> > I think this is a bad idea.  We're clearly low on memory (a memory
> > allocation just failed).  We should free the memory we have allocated
> > to improve the chances of the warning message making it out.
> Not sure if i fully follow you here. We do free the memory. The intention
> was to print a warning message first because, if, potentially, the
> free_vm_area(area) also does some prints it would be a bit messy from the
> point what has been broken first.
> 
> So, could you please clarify what was your concern?

We may need to allocate memory in order to emit the error message.
Your commit message didn't mention the potential confusion, and I think
that is worth adding for a v4.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 20:18       ` Matthew Wilcox
@ 2021-05-09 21:26         ` Uladzislau Rezki
  2021-05-10 10:33           ` Uladzislau Rezki
  0 siblings, 1 reply; 11+ messages in thread
From: Uladzislau Rezki @ 2021-05-09 21:26 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Uladzislau Rezki, Andrew Morton, linux-mm, LKML, Nicholas Piggin,
	Mel Gorman, Hillf Danton, Michal Hocko, Oleksiy Avramchenko,
	Steven Rostedt

On Sun, May 09, 2021 at 09:18:46PM +0100, Matthew Wilcox wrote:
> On Sun, May 09, 2021 at 10:05:19PM +0200, Uladzislau Rezki wrote:
> > On Sun, May 09, 2021 at 08:47:12PM +0100, Matthew Wilcox wrote:
> > > > +++ b/mm/vmalloc.c
> > > > @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > >  	}
> > > >  
> > > >  	if (!area->pages) {
> > > > -		free_vm_area(area);
> > > >  		warn_alloc(gfp_mask, NULL,
> > > >  			   "vmalloc size %lu allocation failure: "
> > > >  			   "page array size %lu allocation failed",
> > > >  			   nr_small_pages * PAGE_SIZE, array_size);
> > > > +		free_vm_area(area);
> > > >  		return NULL;
> > > >  	}
> > > 
> > > I think this is a bad idea.  We're clearly low on memory (a memory
> > > allocation just failed).  We should free the memory we have allocated
> > > to improve the chances of the warning message making it out.
> > Not sure if i fully follow you here. We do free the memory. The intention
> > was to print a warning message first because, if, potentially, the
> > free_vm_area(area) also does some prints it would be a bit messy from the
> > point what has been broken first.
> > 
> > So, could you please clarify what was your concern?
> 
> We may need to allocate memory in order to emit the error message.
>
> Your commit message didn't mention the potential confusion, and I think
> that is worth adding for a v4.
I agree that the commit message should be updated in regard of potential
confusion, because it was the main intention of this patch.

I will upload a v4 tomorrow.

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-09 21:26         ` Uladzislau Rezki
@ 2021-05-10 10:33           ` Uladzislau Rezki
  2021-05-11  1:52             ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Uladzislau Rezki @ 2021-05-10 10:33 UTC (permalink / raw)
  To: Andrew Morton, Matthew Wilcox
  Cc: linux-mm, LKML, Nicholas Piggin, Mel Gorman, Hillf Danton,
	Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Sun, May 09, 2021 at 11:26:41PM +0200, Uladzislau Rezki wrote:
> On Sun, May 09, 2021 at 09:18:46PM +0100, Matthew Wilcox wrote:
> > On Sun, May 09, 2021 at 10:05:19PM +0200, Uladzislau Rezki wrote:
> > > On Sun, May 09, 2021 at 08:47:12PM +0100, Matthew Wilcox wrote:
> > > > > +++ b/mm/vmalloc.c
> > > > > @@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > > >  	}
> > > > >  
> > > > >  	if (!area->pages) {
> > > > > -		free_vm_area(area);
> > > > >  		warn_alloc(gfp_mask, NULL,
> > > > >  			   "vmalloc size %lu allocation failure: "
> > > > >  			   "page array size %lu allocation failed",
> > > > >  			   nr_small_pages * PAGE_SIZE, array_size);
> > > > > +		free_vm_area(area);
> > > > >  		return NULL;
> > > > >  	}
> > > > 
> > > > I think this is a bad idea.  We're clearly low on memory (a memory
> > > > allocation just failed).  We should free the memory we have allocated
> > > > to improve the chances of the warning message making it out.
> > > Not sure if i fully follow you here. We do free the memory. The intention
> > > was to print a warning message first because, if, potentially, the
> > > free_vm_area(area) also does some prints it would be a bit messy from the
> > > point what has been broken first.
> > > 
> > > So, could you please clarify what was your concern?
> > 
> > We may need to allocate memory in order to emit the error message.
> >
> > Your commit message didn't mention the potential confusion, and I think
> > that is worth adding for a v4.
> I agree that the commit message should be updated in regard of potential
> confusion, because it was the main intention of this patch.
> 
> I will upload a v4 tomorrow.
> 
Please find the v4 version of the patch that is in question:

From 7e27e4ac8f299ae244e9e0e90e0292ae2c08d37d Mon Sep 17 00:00:00 2001
From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Date: Sat, 8 May 2021 23:41:21 +0200
Subject: [PATCH v4 1/1] mm/vmalloc: Print a warning message first on failure

When a memory allocation for array of pages are not succeed
emit a warning message as a first step and then perform the
further cleanup.

The reason it should be done in a right order is the clean
up function which is free_vm_area() can potentially also
follow its error paths what can lead to confusion what was
broken first.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index dbc6744400d5..1f664a17d9ea 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2781,11 +2781,11 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	}
 
 	if (!area->pages) {
-		free_vm_area(area);
 		warn_alloc(gfp_mask, NULL,
 			   "vmalloc size %lu allocation failure: "
 			   "page array size %lu allocation failed",
 			   nr_small_pages * PAGE_SIZE, array_size);
+		free_vm_area(area);
 		return NULL;
 	}
 
-- 
2.20.1

--
Vlad Rezki

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-10 10:33           ` Uladzislau Rezki
@ 2021-05-11  1:52             ` Andrew Morton
  2021-05-11  1:58               ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2021-05-11  1:52 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Matthew Wilcox, linux-mm, LKML, Nicholas Piggin, Mel Gorman,
	Hillf Danton, Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Mon, 10 May 2021 12:33:42 +0200 Uladzislau Rezki <urezki@gmail.com> wrote:

> Please find the v4 version of the patch that is in question:
> 
> >From 7e27e4ac8f299ae244e9e0e90e0292ae2c08d37d Mon Sep 17 00:00:00 2001
> From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> Date: Sat, 8 May 2021 23:41:21 +0200
> Subject: [PATCH v4 1/1] mm/vmalloc: Print a warning message first on failure

Added, thanks.

Matthew has a point of course, but I do think that any console driver
which tries to allocate memory within the cotext of printk() is so
pathetic that it isn't worth compromising core code to cater for it...


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-11  1:52             ` Andrew Morton
@ 2021-05-11  1:58               ` Matthew Wilcox
  2021-05-11 12:43                 ` Uladzislau Rezki
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2021-05-11  1:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Uladzislau Rezki, linux-mm, LKML, Nicholas Piggin, Mel Gorman,
	Hillf Danton, Michal Hocko, Oleksiy Avramchenko, Steven Rostedt

On Mon, May 10, 2021 at 06:52:38PM -0700, Andrew Morton wrote:
> On Mon, 10 May 2021 12:33:42 +0200 Uladzislau Rezki <urezki@gmail.com> wrote:
> 
> > Please find the v4 version of the patch that is in question:
> > 
> > >From 7e27e4ac8f299ae244e9e0e90e0292ae2c08d37d Mon Sep 17 00:00:00 2001
> > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > Date: Sat, 8 May 2021 23:41:21 +0200
> > Subject: [PATCH v4 1/1] mm/vmalloc: Print a warning message first on failure
> 
> Added, thanks.
> 
> Matthew has a point of course, but I do think that any console driver
> which tries to allocate memory within the cotext of printk() is so
> pathetic that it isn't worth compromising core code to cater for it...

I'm fine with v4 of this patch, fwiw.  I saw no advantage to the earlier
version, but now that the advantage has been explained, the advantage
outweighs the disadvantage.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure
  2021-05-11  1:58               ` Matthew Wilcox
@ 2021-05-11 12:43                 ` Uladzislau Rezki
  0 siblings, 0 replies; 11+ messages in thread
From: Uladzislau Rezki @ 2021-05-11 12:43 UTC (permalink / raw)
  To: Matthew Wilcox, Andrew Morton
  Cc: Andrew Morton, Uladzislau Rezki, linux-mm, LKML, Nicholas Piggin,
	Mel Gorman, Hillf Danton, Michal Hocko, Oleksiy Avramchenko,
	Steven Rostedt

On Tue, May 11, 2021 at 02:58:18AM +0100, Matthew Wilcox wrote:
> On Mon, May 10, 2021 at 06:52:38PM -0700, Andrew Morton wrote:
> > On Mon, 10 May 2021 12:33:42 +0200 Uladzislau Rezki <urezki@gmail.com> wrote:
> > 
> > > Please find the v4 version of the patch that is in question:
> > > 
> > > >From 7e27e4ac8f299ae244e9e0e90e0292ae2c08d37d Mon Sep 17 00:00:00 2001
> > > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > > Date: Sat, 8 May 2021 23:41:21 +0200
> > > Subject: [PATCH v4 1/1] mm/vmalloc: Print a warning message first on failure
> > 
> > Added, thanks.
> > 
> > Matthew has a point of course, but I do think that any console driver
> > which tries to allocate memory within the cotext of printk() is so
> > pathetic that it isn't worth compromising core code to cater for it...
> 
> I'm fine with v4 of this patch, fwiw.  I saw no advantage to the earlier
> version, but now that the advantage has been explained, the advantage
> outweighs the disadvantage.

Thanks!

The point about an extra memory for printk is correct. From the other
hand i am not able to recall issues related to lack of memory for
printk() to emit a message. At least on our mobile devices i have not
seen such scenarios.

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-05-11 12:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-09 19:38 [PATCH v2 1/2] mm/vmalloc: Switch to bulk allocator in __vmalloc_area_node() Uladzislau Rezki (Sony)
2021-05-09 19:38 ` [PATCH v2 2/2] mm/vmalloc: Print a warning message first on failure Uladzislau Rezki (Sony)
2021-05-09 19:46   ` Uladzislau Rezki
2021-05-09 19:47   ` Matthew Wilcox
2021-05-09 20:05     ` Uladzislau Rezki
2021-05-09 20:18       ` Matthew Wilcox
2021-05-09 21:26         ` Uladzislau Rezki
2021-05-10 10:33           ` Uladzislau Rezki
2021-05-11  1:52             ` Andrew Morton
2021-05-11  1:58               ` Matthew Wilcox
2021-05-11 12:43                 ` Uladzislau Rezki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).