diff mbox series

[v2,2/2] mm/vmalloc: Print a warning message first on failure

Message ID 20210509193844.2562-2-urezki@gmail.com (mailing list archive)
State New
Headers show
Series [v2,1/2] mm/vmalloc: Switch to bulk allocator in __vmalloc_area_node() | expand

Commit Message

Uladzislau Rezki May 9, 2021, 7:38 p.m. UTC
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(-)

Comments

Uladzislau Rezki May 9, 2021, 7:46 p.m. UTC | #1
> 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;
 	}
Matthew Wilcox May 9, 2021, 7:47 p.m. UTC | #2
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.
Uladzislau Rezki May 9, 2021, 8:05 p.m. UTC | #3
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
Matthew Wilcox May 9, 2021, 8:18 p.m. UTC | #4
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.
Uladzislau Rezki May 9, 2021, 9:26 p.m. UTC | #5
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
Uladzislau Rezki May 10, 2021, 10:33 a.m. UTC | #6
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;
 	}
Andrew Morton May 11, 2021, 1:52 a.m. UTC | #7
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...
Matthew Wilcox May 11, 2021, 1:58 a.m. UTC | #8
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.
Uladzislau Rezki May 11, 2021, 12:43 p.m. UTC | #9
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
diff mbox series

Patch

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;
 	}