On 2023/3/6 15:51, Michal Hocko wrote:
[Cc couple of more people recently involved with vmalloc code] On Sun 05-03-23 13:30:35, Gao Xiang wrote:My knowledge of this is somewhat limited, however, since vmalloc already supported __GFP_NOFAIL in commit 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL"). __GFP_NOFAIL could trigger the following stack and allocate high-order pages when CONFIG_HAVE_ARCH_HUGE_VMALLOC is enabled: __alloc_pages+0x1cb/0x5b0 mm/page_alloc.c:5549 alloc_pages+0x1aa/0x270 mm/mempolicy.c:2286 vm_area_alloc_pages mm/vmalloc.c:2989 [inline] __vmalloc_area_node mm/vmalloc.c:3057 [inline] __vmalloc_node_range+0x978/0x13c0 mm/vmalloc.c:3227 kvmalloc_node+0x156/0x1a0 mm/util.c:606 kvmalloc include/linux/slab.h:737 [inline] kvmalloc_array include/linux/slab.h:755 [inline] kvcalloc include/linux/slab.h:760 [inline] (codebase: Linux 6.2-rc2) Don't warn such cases since high-order pages with __GFP_NOFAIL is somewhat legel.OK, this is definitely a bug and it seems my 9376130c390a was incomplete because it hasn't covered the high order case. Not sure how that happened but removing the warning is not the right thing to do here. The higher order allocation is an optimization rather than a must. So it is perfectly fine to fail that allocation and retry rather than go into a very expensive and potentially impossible higher order allocation that must not fail. The proper fix should look like this unless I am missing something. I would appreciate another pair of eyes on this because I am not fully familiar with the high order optimization part much.
I'm fine with the fix. Although I'm not familiar with such vmalloc allocation, I thought about this possibility as well. The original issue was: https://lore.kernel.org/r/0000000000007796bd05f1852ec2@xxxxxxxxxx which I used kvcalloc with __GFP_NOFAIL but it warned, and I made a fix (which now seems wrong) to use kcalloc() but it now warns the same: https://lore.kernel.org/r/00000000000072eb6505f376dd4b@xxxxxxxxxx And I then realized it's a bug in kvmalloc() with __GFP_NOFAIL... Thanks, Gao Xiang
Thanks! --- diff --git a/mm/vmalloc.c b/mm/vmalloc.c index ef910bf349e1..a8aa2765618a 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2883,6 +2883,8 @@ vm_area_alloc_pages(gfp_t gfp, int nid, unsigned int order, unsigned int nr_pages, struct page **pages) { unsigned int nr_allocated = 0; + gfp_t alloc_gfp = gfp; + bool nofail = false; struct page *page; int i;@@ -2931,20 +2933,30 @@ vm_area_alloc_pages(gfp_t gfp, int nid,if (nr != nr_pages_request) break; } + } else { + alloc_gfp &= ~__GFP_NOFAIL; + nofail = true; }/* High-order pages or fallback path if "bulk" fails. */- while (nr_allocated < nr_pages) { if (fatal_signal_pending(current)) break;if (nid == NUMA_NO_NODE)- page = alloc_pages(gfp, order); + page = alloc_pages(alloc_gfp, order); else - page = alloc_pages_node(nid, gfp, order); - if (unlikely(!page)) - break; + page = alloc_pages_node(nid, alloc_gfp, order); + if (unlikely(!page)) { + if (!nofail) + break; + + /* fall back to the zero order allocations */ + alloc_gfp |= __GFP_NOFAIL; + order = 0; + continue; + } + /* * Higher order allocations must be able to be treated as * indepdenent small pages by callers (as they can with
