Re: [failures] mm-vmalloc-print-a-warning-message-first-on-failure.patch removed from -mm tree

[Mel Gorman <mgorman@suse.de>]

On Thu, May 13, 2021 at 10:18:51PM +0200, Uladzislau Rezki wrote:
> <SNIP>
>
> From the trace i get:
> 
> <snip>
> [    0.243916] RIP: 0010:__alloc_pages+0x11e/0x310
> [    0.243916] Code: 84 c0 0f 85 02 01 00 00 89 d8 48 8b 54 24 08 8b 74 24 1c c1 e8 0c 83 e0 01 88 44 24 20 48 8b 04 24 48 85 d2 0f 85 71 01 00 00 <3b> 70 08 0f 82 68 01 00 00 48 89 44 24 10 48 8b 00 89 da 81 e2 00
> [    0.243916] RSP: 0000:ffffffffae803c38 EFLAGS: 00010246
> [    0.243916] RAX: 0000000000001cc0 RBX: 0000000000002102 RCX: 0000000000000004
> [    0.243916] RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000002102
> [    0.243916] RBP: 0000000000000000 R08: 0000000000000000 R09: c0000000ffffdfff
> [    0.243916] R10: 0000000000000001 R11: ffffffffae803ac0 R12: 0000000000000000
> [    0.243916] R13: 0000000000002102 R14: 0000000000000001 R15: ffffa0938000d000
> [    0.243916] FS:  0000000000000000(0000) GS:ffff893ab7c00000(0000) knlGS:0000000000000000
> [    0.243916] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.243916] CR2: 0000000000001cc8 CR3: 0000000176e10000 CR4: 00000000000006b0
> [    0.243916] Call Trace:
> [    0.243916]  __alloc_pages_bulk+0xaa1/0xb50
> <snip>
> 
> (gdb) l *__alloc_pages+0x11e
> 0xffffffff8129d87e is in __alloc_pages (./include/linux/mmzone.h:1095).
> 1090            return zoneref->zone;
> 1091    }
> 1092
> 1093    static inline int zonelist_zone_idx(struct zoneref *zoneref)
> 1094    {
> 1095            return zoneref->zone_idx;
> 1096    }
> 1097
> 1098    static inline int zonelist_node_idx(struct zoneref *zoneref)
> 1099    {
> (gdb)
> 
> Seems like "zoneref" refers to invalid address.
> 
> Thoughts?

I have not previously read the patch but there are a few concerns and it's
probably just as well this blew up early. The bulk allocator assumes a
valid node but the patch can send in NUMA_NO_NODE (-1). On the high-order
path alloc_pages_node is used which checks nid == NUMA_NO_NODE. Also,
area->pages is not necessarily initialised so that could be interpreted
as a partially populated array so minmally you need.

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index f3c5dd4ccc5b9..b638ff31b07e1 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2792,6 +2792,9 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	page_order = vm_area_page_order(area);
 
 	if (!page_order) {
+		memset(area->pages, 0, array_size);
+		if (node == NUMA_NO_NODE)
+			node = numa_mem_id();
 		area->nr_pages = __alloc_pages_bulk(gfp_mask, node,
 			NULL, nr_small_pages, NULL, area->pages);
 	} else {

However, the high-order path also looks suspicious. area->nr_pages is
advanced before the allocation attempt so in the event alloc_pages_node()
returns NULL prematurely, area->nr_pages does not reflect the number of
pages allocated so that needs examination.

As an aside, where or what is test_vmalloc.sh? It appears to have been
used a few times but it's not clear it's representative so are you aware
of workloads that are vmalloc-intensive? It does not matter for the
patch as such but it would be nice to know examples of vmalloc-intensive
workloads because I cannot recall a time during the last few years where
I saw vmalloc.c high in profiles.

-- 
Mel Gorman
SUSE Labs