diff mbox series

mm/page_alloc: check return value of memblock_alloc_node_nopanic()

Message ID 1547621481-8374-1-git-send-email-rppt@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series mm/page_alloc: check return value of memblock_alloc_node_nopanic() | expand

Commit Message

Mike Rapoport Jan. 16, 2019, 6:51 a.m. UTC
There are two early memory allocations that use
memblock_alloc_node_nopanic() and do not check its return value.
While this happens very early during boot and chances that the allocation
will fail are diminishing, it is still worth to have proper checks for the
allocation errors.

Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
 mm/page_alloc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

William Kucharski Jan. 17, 2019, 10:19 a.m. UTC | #1
This seems very reasonable, but if the code is just going to panic if the allocation fails, why not call memblock_alloc_node() instead?

If there is a reason we'd prefer to call memblock_alloc_node_nopanic(), I'd like to see pgdat->nodeid printed in the panic message as well.
Mike Rapoport Jan. 17, 2019, 11:26 a.m. UTC | #2
On Thu, Jan 17, 2019 at 03:19:35AM -0700, William Kucharski wrote:
> 
> This seems very reasonable, but if the code is just going to panic if the
> allocation fails, why not call memblock_alloc_node() instead?

I've sent patches [1] that remove panic() from memblock_alloc*() and drop
_nopanic variants. After they will be (hopefully) merged,
memblock_alloc_node() will return NULL on error.
 
> If there is a reason we'd prefer to call memblock_alloc_node_nopanic(),
> I'd like to see pgdat->nodeid printed in the panic message as well.

Sure.

[1] https://lore.kernel.org/lkml/1547646261-32535-1-git-send-email-rppt@linux.ibm.com/
William Kucharski Jan. 17, 2019, 1:34 p.m. UTC | #3
> On Jan 17, 2019, at 4:26 AM, Mike Rapoport <rppt@linux.ibm.com> wrote:
> 
> On Thu, Jan 17, 2019 at 03:19:35AM -0700, William Kucharski wrote:
>> 
>> This seems very reasonable, but if the code is just going to panic if the
>> allocation fails, why not call memblock_alloc_node() instead?
> 
> I've sent patches [1] that remove panic() from memblock_alloc*() and drop
> _nopanic variants. After they will be (hopefully) merged,
> memblock_alloc_node() will return NULL on error.
> 
>> If there is a reason we'd prefer to call memblock_alloc_node_nopanic(),
>> I'd like to see pgdat->nodeid printed in the panic message as well.
> 
> Sure.

Thanks for the quick response.

Reviewed-by: William Kucharski <william.kucharski@oracle.com>
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d295c9bc01a8..7801accbe02a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6376,10 +6376,14 @@  static void __ref setup_usemap(struct pglist_data *pgdat,
 {
 	unsigned long usemapsize = usemap_size(zone_start_pfn, zonesize);
 	zone->pageblock_flags = NULL;
-	if (usemapsize)
+	if (usemapsize) {
 		zone->pageblock_flags =
 			memblock_alloc_node_nopanic(usemapsize,
 							 pgdat->node_id);
+		if (!zone->pageblock_flags)
+			panic("Failed to allocate %ld bytes for zone %s pageblock flags\n",
+			      usemapsize, zone->name);
+	}
 }
 #else
 static inline void setup_usemap(struct pglist_data *pgdat, struct zone *zone,
@@ -6609,6 +6613,9 @@  static void __ref alloc_node_mem_map(struct pglist_data *pgdat)
 		end = ALIGN(end, MAX_ORDER_NR_PAGES);
 		size =  (end - start) * sizeof(struct page);
 		map = memblock_alloc_node_nopanic(size, pgdat->node_id);
+		if (!map)
+			panic("Failed to allocate %ld bytes for memory map\n",
+			      size);
 		pgdat->node_mem_map = map + offset;
 	}
 	pr_debug("%s: node %d, pgdat %08lx, node_mem_map %08lx\n",