Patchwork [1/6] sparc: remove dependency on __GFP_NOFAIL

login
register
mail settings
Submitter David Rientjes
Date July 21, 2010, 2:44 a.m.
Message ID <alpine.DEB.2.00.1007201938100.8728@chino.kir.corp.google.com>
Download mbox | patch
Permalink /patch/59399/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

David Rientjes - July 21, 2010, 2:44 a.m.
The kmalloc() in mdesc_kmalloc() is failable, so remove __GFP_NOFAIL from
its mask.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 arch/sparc/kernel/mdesc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 21, 2010, 3:31 a.m.
From: David Rientjes <rientjes@google.com>
Date: Tue, 20 Jul 2010 19:44:53 -0700 (PDT)

> The kmalloc() in mdesc_kmalloc() is failable, so remove __GFP_NOFAIL from
> its mask.
> 
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Rientjes <rientjes@google.com>

The __GFP_NOFAIL is there intentionally.

The code above this, in the cases where the machine description is
dynamically updated by the hypervisor at run time, long after boot,
has no failure handling.

We absolutely must accept the machine descriptor update and fetch it
from the hypervisor into a new buffer.

Please don't remove this.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rientjes - July 21, 2010, 9:41 a.m.
On Tue, 20 Jul 2010, David Miller wrote:

> > The kmalloc() in mdesc_kmalloc() is failable, so remove __GFP_NOFAIL from
> > its mask.
> > 
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> The __GFP_NOFAIL is there intentionally.
> 
> The code above this, in the cases where the machine description is
> dynamically updated by the hypervisor at run time, long after boot,
> has no failure handling.
> 
> We absolutely must accept the machine descriptor update and fetch it
> from the hypervisor into a new buffer.
> 
> Please don't remove this.
> 

Ok, fair enough.  I was convinced by the error handling in both 
mdesc_update() and mdesc_kmallloc() that this was a failable allocation, 
but I understand how mdesc_update() must succeed given your description.  
We can remove those branches from those two functions, though, since 
__GFP_NOFAIL will always succeed before returning.

I'm planning on replacing __GFP_NOFAIL with a __GFP_KILLABLE flag that 
will use all of the page allocator's capabilities (direct reclaim, memory 
compaction for order > 0, and the oom killer) before failing.  Then, 
existing __GFP_NOFAIL users can use

	do {
		page = alloc_page(GFP_KERNEL | __GFP_KILLABLE);
	} while (!page);

to remove several branches from the page allocator that we'll no longer 
need.  I'll do this in phase two and make sure to convert this instance to 
do that.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -134,7 +134,7 @@  static struct mdesc_handle *mdesc_kmalloc(unsigned int mdesc_size)
 		       sizeof(struct mdesc_hdr) +
 		       mdesc_size);
 
-	base = kmalloc(handle_size + 15, GFP_KERNEL | __GFP_NOFAIL);
+	base = kmalloc(handle_size + 15, GFP_KERNEL);
 	if (base) {
 		struct mdesc_handle *hp;
 		unsigned long addr;