On 2024/5/9 11:48, Barry Song wrote:
On Thu, May 9, 2024 at 3:33 PM Hailong Liu <hailong.liu@xxxxxxxx> wrote:
On Thu, 09. May 14:20, Barry Song wrote:
On Thu, May 9, 2024 at 12:58 AM <hailong.liu@xxxxxxxx> wrote:
From: "Hailong.Liu" <hailong.liu@xxxxxxxx>
Commit a421ef303008 ("mm: allow !GFP_KERNEL allocations for kvmalloc")
includes support for __GFP_NOFAIL, but it presents a conflict with
commit dd544141b9eb ("vmalloc: back off when the current task is
OOM-killed"). A possible scenario is as belows:
process-a
kvcalloc(n, m, GFP_KERNEL | __GFP_NOFAIL)
__vmalloc_node_range()
__vmalloc_area_node()
vm_area_alloc_pages()
--> oom-killer send SIGKILL to process-a
if (fatal_signal_pending(current)) break;
--> return NULL;
to fix this, do not check fatal_signal_pending() in vm_area_alloc_pages()
if __GFP_NOFAIL set.
Reported-by: Oven <liyangouwen1@xxxxxxxx>
Signed-off-by: Hailong.Liu <hailong.liu@xxxxxxxx>
---
mm/vmalloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 6641be0ca80b..2f359d08bf8d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3560,7 +3560,7 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
/* High-order pages or fallback path if "bulk" fails. */
while (nr_allocated < nr_pages) {
- if (fatal_signal_pending(current))
+ if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
break;
why not !nofail ?
if order = 0, nofail would not be set true in bulk allocator. in such a case,
it is still possible to break early
This seems a correct fix, but it undermines the assumption made in
commit dd544141b9eb
("vmalloc: back off when the current task is OOM-killed")
"
This may trigger some hidden problems, when caller does not handle
vmalloc failures, or when rollaback after failed vmalloc calls own
vmallocs inside. However all of these scenarios are incorrect: vmalloc
does not guarantee successful allocation, it has never been called with
__GFP_NOFAIL and threfore either should not be used for any rollbacks or
should handle such errors correctly and not lead to critical failures.
"
If a significant kvmalloc operation is performed with the NOFAIL flag, it risks
reverting the fix intended to address the OOM-killer issue in commit
dd544141b9eb.
Should we indeed permit the NOFAIL flag for large kvmalloc allocations?
IMO, if we encounter this issue, it should be fixed by the
caller, not here.
I agree. but could we WARN_ON a large kvmalloc(NOFAIL) allocation?
For me, kvmalloc(~24k) sounds good to handle extreme cases (for extreme
compression), only specific users for file archiving (and fuzzers) are
expected to use this configuration.
Anyway, it's just my own ideas.
Thanks,
Gao Xiang
Thanks
Barry
--
Best Regards,
Hailong.
