Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752367AbbEFXaU (ORCPT ); Wed, 6 May 2015 19:30:20 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:56443 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752322AbbEFXaR (ORCPT ); Wed, 6 May 2015 19:30:17 -0400 Date: Wed, 6 May 2015 16:30:16 -0700 From: Andrew Morton To: Tony Luck Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 2/3] mm/memblock: Allocate boot time data structures from mirrored memory Message-Id: <20150506163016.a2d79f89abc7543cb80307ac@linux-foundation.org> In-Reply-To: References: X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4761 Lines: 179 On Tue, 3 Feb 2015 14:38:02 -0800 Tony Luck wrote: > Try to allocate all boot time kernel data structures from mirrored > memory. If we run out of mirrored memory print warnings, but fall > back to using non-mirrored memory to make sure that we still boot. > > ... > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h > index 1d448879caae..20bf3dfab564 100644 > --- a/include/linux/memblock.h > +++ b/include/linux/memblock.h > @@ -22,6 +22,7 @@ > > /* Definition of memblock flags. */ > #define MEMBLOCK_HOTPLUG 0x1 /* hotpluggable region */ > +#define MEMBLOCK_MIRROR 0x2 /* mirrored region */ It would be nice to make these an enum. Then all those literal "0"'s which were added in [1/3] become MEMBLOCK_NONE, which is self-documenting. > > ... > > +static inline bool memblock_is_mirror(struct memblock_region *m) > +{ > + return m->flags & MEMBLOCK_MIRROR; > +} > + > > ... > > +u32 __init_memblock memblock_has_mirror(void) > +{ > + return memblock_have_mirror ? MEMBLOCK_MIRROR : 0; > +} hm, these are very similar. But I guess they're different enough. Gramatically, a function called "memblock_has_mirror()" should return a bool. This guy is misnamed. "memblock_mirror_flag()"? > /* inline so we don't get a warning when pr_debug is compiled out */ > static __init_memblock const char * > memblock_type_name(struct memblock_type *type) > @@ -257,8 +263,19 @@ phys_addr_t __init_memblock memblock_find_in_range(phys_addr_t start, > phys_addr_t end, phys_addr_t size, > phys_addr_t align) > { > - return memblock_find_in_range_node(size, align, start, end, > + phys_addr_t ret; > + u32 flag = memblock_has_mirror(); > + > + ret = memblock_find_in_range_node(size, align, start, end, > + NUMA_NO_NODE, flag); > + > + if (!ret && flag) { > + pr_warn("Could not allocate %lld bytes of mirrored memory\n", size); This printk will warn on some configs. Print a phys_addr_t with %pap. I think. See huge comment over lib/vsprintf.c:pointer(). There are other instances of this. > + ret = memblock_find_in_range_node(size, align, start, end, > NUMA_NO_NODE, 0); > + } > + > + return ret; > } > > ... > > phys_addr_t __init memblock_alloc_nid(phys_addr_t size, phys_addr_t align, int nid) > { > - return memblock_alloc_base_nid(size, align, MEMBLOCK_ALLOC_ACCESSIBLE, nid, 0); > + u32 flag = memblock_has_mirror(); > + phys_addr_t ret; > + > +again: > + ret = memblock_alloc_base_nid(size, align, MEMBLOCK_ALLOC_ACCESSIBLE, nid, flag); > + > + if (!ret && flag) { > + flag = 0; > + goto again; > + } What's going on here? This is where we're falling back to non-mirrored. But it's happening silently? Should it warn, or is that handled elsewhere? This function isn't specific to mirrored memory - for any future flags, falling back to flags==0 may not be the desired behavior. What do we do then? I guess if (!ret && (flag & MEMBLOCK_MIRROR)) ( flag &= ~MEMBLOCK_MIRROR; goto again; yes? That can be done later if needed, I suppose. > + return ret; > } > > > ... > > @@ -1181,13 +1232,13 @@ static void * __init memblock_virt_alloc_internal( > > again: > alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > - nid, 0); > + nid, flag); > if (alloc) > goto done; > > if (nid != NUMA_NO_NODE) { > alloc = memblock_find_in_range_node(size, align, min_addr, > - max_addr, NUMA_NO_NODE, 0); > + max_addr, NUMA_NO_NODE, flag); > if (alloc) > goto done; > } > @@ -1195,10 +1246,15 @@ again: > if (min_addr) { > min_addr = 0; > goto again; > - } else { > - goto error; > } > > + if (flag) { > + flag = 0; > + pr_warn("Could not allocate %lld bytes of mirrored memory\n", size); printk warning. Please don't torture people who use 80-col displays! > + goto again; > + } > + > + return NULL; > > ... > > @@ -37,11 +37,19 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align, > { > void *ptr; > u64 addr; > + u32 flag = memblock_has_mirror(); > > if (limit > memblock.current_limit) > limit = memblock.current_limit; > > - addr = memblock_find_in_range_node(size, align, goal, limit, nid, 0); > +again: > + addr = memblock_find_in_range_node(size, align, goal, limit, nid, flag); > + > + if (flag && !addr) { > + flag = 0; > + pr_warn("Could not allocate %lld bytes of mirrored memory\n", size); dittoes. > + goto again; > + } > if (!addr) > return NULL; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/