[PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Pekka Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
BUG_ON() calls.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slub.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 7ab54ec..55216b4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2109,9 +2109,8 @@ static void early_kmem_cache_node_alloc(gfp_t gfpflags, int node)
 
 	BUG_ON(kmalloc_caches->size < sizeof(struct kmem_cache_node));
 
-	page = new_slab(kmalloc_caches, gfpflags, node);
+	page = new_slab(kmalloc_caches, gfpflags | GFP_PANIC, node);
 
-	BUG_ON(!page);
 	if (page_to_nid(page) != node) {
 		printk(KERN_ERR "SLUB: Unable to allocate memory from "
 				"node %d\n", node);
@@ -4367,10 +4366,9 @@ static struct kset *slab_kset;
  */
 static char *create_unique_id(struct kmem_cache *s)
 {
-	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
-	char *p = name;
+	char *name, *p;
 
-	BUG_ON(!name);
+	p = name = kmalloc(ID_STR_LENGTH, GFP_KERNEL | GFP_PANIC);
 
 	*p++ = ':';
 	/*
-- 
1.5.6.3

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Cyrill Gorcunov]

[Pekka Enberg - Fri, May 08, 2009 at 06:10:29PM +0300]
| From: Pekka Enberg <penberg@cs.helsinki.fi>
| 
| This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
| BUG_ON() calls.
| 
| Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
| ---
|  mm/slub.c |    8 +++-----
|  1 files changed, 3 insertions(+), 5 deletions(-)
|

If the idea will be accepted -- I add 3 callsites on x86
area too :)

	-- Cyrill

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

> This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> BUG_ON() calls.

Yirks this is not only early boot code. create_unique_id may be executed
whenever a slab is created.

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Pekka Enberg]

On Fri, 2009-05-08 at 11:28 -0400, Christoph Lameter wrote:
> On Fri, 8 May 2009, Pekka Enberg wrote:
> 
> > This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> > BUG_ON() calls.
> 
> Yirks this is not only early boot code. create_unique_id may be executed
> whenever a slab is created.

Argh. We should do -ENOMEM there then. I'll drop that hunk.

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Peter Zijlstra]

On Fri, 2009-05-08 at 18:36 +0300, Pekka Enberg wrote:
> On Fri, 2009-05-08 at 11:28 -0400, Christoph Lameter wrote:
> > On Fri, 8 May 2009, Pekka Enberg wrote:
> > 
> > > This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> > > BUG_ON() calls.
> > 
> > Yirks this is not only early boot code. create_unique_id may be executed
> > whenever a slab is created.
> 
> Argh. We should do -ENOMEM there then. I'll drop that hunk.

BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));

?

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Pekka Enberg]

Hi Peter,

On Fri, 2009-05-08 at 11:28 -0400, Christoph Lameter wrote:
> > > On Fri, 8 May 2009, Pekka Enberg wrote:
> > > 
> > > > This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> > > > BUG_ON() calls.
> > > 
> > > Yirks this is not only early boot code. create_unique_id may be executed
> > > whenever a slab is created.

On Fri, 2009-05-08 at 18:36 +0300, Pekka Enberg wrote:
> > Argh. We should do -ENOMEM there then. I'll drop that hunk.

On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
> BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));

There's no technical reason not to use GFP_PANIC when system_state !=
STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
(and BUG_ON) is IMHO too harsh for create_unique_id().

			Pekka

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Peter Zijlstra]

On Fri, 2009-05-08 at 18:45 +0300, Pekka Enberg wrote:

> On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
> > BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));
> 
> There's no technical reason not to use GFP_PANIC when system_state !=
> STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
> (and BUG_ON) is IMHO too harsh for create_unique_id().

Shouldn't we handle every allocation failure after booting?

I think it _is_ a bug to panic on allocation failures once we're
running.

I'm really not very fond of __GFP_PANIC as it stands, it seems to
suggest its OK to ignore allocation failures, which would bring us back
to the original UNIX error handling :/

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Cyrill Gorcunov]

[Peter Zijlstra - Fri, May 08, 2009 at 05:50:58PM +0200]
| On Fri, 2009-05-08 at 18:45 +0300, Pekka Enberg wrote:
| 
| > On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
| > > BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));
| > 
| > There's no technical reason not to use GFP_PANIC when system_state !=
| > STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
| > (and BUG_ON) is IMHO too harsh for create_unique_id().
| 
| Shouldn't we handle every allocation failure after booting?

Definitely

| 
| I think it _is_ a bug to panic on allocation failures once we're
| running.
|

But Peter I believe there was no suggestion to use GFP_PANIC everywhere
to get rid of error handling. But rather to use it in case if kmalloc is
followed by BUG_ON.

| 
| I'm really not very fond of __GFP_PANIC as it stands, it seems to
| suggest its OK to ignore allocation failures, which would bring us back
| to the original UNIX error handling :/
| 
	-- Cyrill

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Peter Zijlstra]

On Fri, 2009-05-08 at 20:15 +0400, Cyrill Gorcunov wrote:
> [Peter Zijlstra - Fri, May 08, 2009 at 05:50:58PM +0200]
> | On Fri, 2009-05-08 at 18:45 +0300, Pekka Enberg wrote:
> | 
> | > On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
> | > > BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));
> | > 
> | > There's no technical reason not to use GFP_PANIC when system_state !=
> | > STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
> | > (and BUG_ON) is IMHO too harsh for create_unique_id().
> | 
> | Shouldn't we handle every allocation failure after booting?
> 
> Definitely
> 
> | 
> | I think it _is_ a bug to panic on allocation failures once we're
> | running.
> |
> 
> But Peter I believe there was no suggestion to use GFP_PANIC everywhere
> to get rid of error handling. But rather to use it in case if kmalloc is
> followed by BUG_ON.

Well, what I'm saying is that that either is a genuine bug we should
fix, or its boot code, which is exactly what my assertion above tests
for.

I really believe GFP_PANIC usage outside of system_state ==
STATE_BOOTING is a bug we should fix. Not create a better syntax for.

What code outside of booting is allowed to bring down the kernel on an
allocation failure?

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Christoph Lameter]

On Fri, 8 May 2009, Peter Zijlstra wrote:

> Well, what I'm saying is that that either is a genuine bug we should
> fix, or its boot code, which is exactly what my assertion above tests
> for.
>
> I really believe GFP_PANIC usage outside of system_state ==
> STATE_BOOTING is a bug we should fix. Not create a better syntax for.

I agree.

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Cyrill Gorcunov]

[Peter Zijlstra - Fri, May 08, 2009 at 06:31:07PM +0200]
| On Fri, 2009-05-08 at 20:15 +0400, Cyrill Gorcunov wrote:
| > [Peter Zijlstra - Fri, May 08, 2009 at 05:50:58PM +0200]
| > | On Fri, 2009-05-08 at 18:45 +0300, Pekka Enberg wrote:
| > | 
| > | > On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
| > | > > BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));
| > | > 
| > | > There's no technical reason not to use GFP_PANIC when system_state !=
| > | > STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
| > | > (and BUG_ON) is IMHO too harsh for create_unique_id().
| > | 
| > | Shouldn't we handle every allocation failure after booting?
| > 
| > Definitely
| > 
| > | 
| > | I think it _is_ a bug to panic on allocation failures once we're
| > | running.
| > |
| > 
| > But Peter I believe there was no suggestion to use GFP_PANIC everywhere
| > to get rid of error handling. But rather to use it in case if kmalloc is
| > followed by BUG_ON.
| 
| Well, what I'm saying is that that either is a genuine bug we should
| fix, or its boot code, which is exactly what my assertion above tests
| for.

Yes, I agree and didn't argue against actually (but most probably I
was not precise in words). There is no protection of using it wrong
or buggy way (to eliminate error paths but just panic). By adding
STATE_BOOTING we seem to protect ourself from such a case but then
we better rename it to something like GFP_BOOT_PANIC maybe? Since
from plain GFP_PANIC it's not clear, hmm?

| 
| I really believe GFP_PANIC usage outside of system_state ==
| STATE_BOOTING is a bug we should fix. Not create a better syntax for.
| 
| What code outside of booting is allowed to bring down the kernel on an
| allocation failure?
| 

	-- Cyrill

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, 2009-05-08 at 20:15 +0400, Cyrill Gorcunov wrote:
> > [Peter Zijlstra - Fri, May 08, 2009 at 05:50:58PM +0200]
> > | On Fri, 2009-05-08 at 18:45 +0300, Pekka Enberg wrote:
> > | 
> > | > On Fri, 2009-05-08 at 17:42 +0200, Peter Zijlstra wrote:
> > | > > BUG_ON((gfp & __GFP_PANIC) && (system_state != STATE_BOOTING));
> > | > 
> > | > There's no technical reason not to use GFP_PANIC when system_state !=
> > | > STATE_BOOTING so I don't think it's needed. It's just that GFP_PANIC
> > | > (and BUG_ON) is IMHO too harsh for create_unique_id().
> > | 
> > | Shouldn't we handle every allocation failure after booting?
> > 
> > Definitely
> > 
> > | 
> > | I think it _is_ a bug to panic on allocation failures once we're
> > | running.
> > |
> > 
> > But Peter I believe there was no suggestion to use GFP_PANIC everywhere
> > to get rid of error handling. But rather to use it in case if kmalloc is
> > followed by BUG_ON.
> 
> Well, what I'm saying is that that either is a genuine bug we 
> should fix, or its boot code, which is exactly what my assertion 
> above tests for.

This is about boot code - and only about boot code.

This is a really simple thing. Replace existing x86 platform code 
early boot allocaton patterns of:

	x = alloc(GFP_KERNEL, ...);
	BUG_ON(!x);

	y = alloc(GFP_KERNEL, ...);
	BUG_ON(!y);

With:

	x = alloc(GFP_KERNEL | __GFP_PANIC, ...);

	y = alloc(GFP_KERNEL | __GFP_PANIC, ...);

it makes code shorter, smaller and easier to read. Nothing more, 
nothing less. And yes, i agree that we should disallow this after 
bootup has finished - but the boot-alloc case Cyrill and me would 
like to handle via this is still fully valid.

	Ingo

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Pekka Enberg]

On Fri, 2009-05-08 at 11:28 -0400, Christoph Lameter wrote:
> On Fri, 8 May 2009, Pekka Enberg wrote:
> 
> > This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> > BUG_ON() calls.
> 
> Yirks this is not only early boot code. create_unique_id may be executed
> whenever a slab is created.

Updated patch here:

Subject: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations
From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch converts SLUB early-boot code to use GFP_PANIC instead of BUG_ON().

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slub.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 7ab54ec..2cdf731 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2109,9 +2109,8 @@ static void early_kmem_cache_node_alloc(gfp_t gfpflags, int node)
 
 	BUG_ON(kmalloc_caches->size < sizeof(struct kmem_cache_node));
 
-	page = new_slab(kmalloc_caches, gfpflags, node);
+	page = new_slab(kmalloc_caches, gfpflags | GFP_PANIC, node);
 
-	BUG_ON(!page);
 	if (page_to_nid(page) != node) {
 		printk(KERN_ERR "SLUB: Unable to allocate memory from "
 				"node %d\n", node);
-- 
1.5.6.3

Re: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On Fri, 2009-05-08 at 11:28 -0400, Christoph Lameter wrote:
> > On Fri, 8 May 2009, Pekka Enberg wrote:
> > 
> > > This patch converts SLUB early-boot code to use GFP_PANIC instead of explicit
> > > BUG_ON() calls.
> > 
> > Yirks this is not only early boot code. create_unique_id may be executed
> > whenever a slab is created.
> 
> Updated patch here:
> 
> Subject: [PATCH 2/2] SLUB: Use GFP_PANIC for early-boot allocations
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This patch converts SLUB early-boot code to use GFP_PANIC instead of BUG_ON().
> 
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

thanks Pekka!

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo