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