From: Gao Xiang <hsiangkao@linux.alibaba.com>
To: Barry Song <21cnbao@gmail.com>, hailong.liu@oppo.com
Cc: akpm@linux-foundation.org, urezki@gmail.com, hch@infradead.org,
lstoakes@gmail.com, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, xiang@kernel.org, chao@kernel.org,
Oven <liyangouwen1@oppo.com>
Subject: Re: [RFC PATCH] mm/vmalloc: fix vmalloc which may return null if called with __GFP_NOFAIL
Date: Thu, 9 May 2024 10:39:31 +0800 [thread overview]
Message-ID: <20d782ad-c059-4029-9c75-0ef278c98d81@linux.alibaba.com> (raw)
In-Reply-To: <CAGsJ_4xN0MBz_73wUvMp74upd9SaQ+TCRJufEj26Y619Rtr7Zw@mail.gmail.com>
Hi,
On 2024/5/9 10:20, Barry Song wrote:
> On Thu, May 9, 2024 at 12:58 AM <hailong.liu@oppo.com> wrote:
>>
>> From: "Hailong.Liu" <hailong.liu@oppo.com>
>>
>> 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@oppo.com>
>> Signed-off-by: Hailong.Liu <hailong.liu@oppo.com>
>> ---
>> 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 ?
>
> 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?
Just from my perspective, I don't really care about kmalloc, vmalloc
or kvmalloc (__GFP_NOFAIL). I even don't care if it returns three
order-0 pages or a high-order page. I just would like to need a
virtual consecutive buffer (even it works slowly.) with __GFP_NOFAIL.
Because in some cases, writing fallback code may be tough and hard to
test if such fallback path is correct since it only triggers in extreme
workloads, and even such buffers are just used in a very short lifetime.
Also see other FS discussion of __GFP_NOFAIL, e.g.
https://lore.kernel.org/all/ZcUQfzfQ9R8X0s47@tiehlicka/
In the worst cases, it usually just needs < 5 order-0 pages (for many
cases it only needs one page), but with kmalloc it will trigger WARN
if it occurs to > order-1 allocation. as I mentioned before.
With my limited understanding I don't see why it could any problem with
kvmalloc(__GFP_NOFAIL) since it has no difference of kmalloc(GFP_NOFAIL)
with order-0 allocation.
Thanks,
Gao XIang
next prev parent reply other threads:[~2024-05-09 2:39 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 12:58 [RFC PATCH] mm/vmalloc: fix vmalloc which may return null if called with __GFP_NOFAIL hailong.liu
2024-05-08 13:41 ` Gao Xiang
2024-05-08 14:13 ` Gao Xiang
2024-05-08 14:43 ` Hailong Liu
2024-05-08 15:10 ` Gao Xiang
2024-05-08 15:31 ` Hailong Liu
2024-05-08 15:40 ` Gao Xiang
2024-05-09 1:30 ` Hailong Liu
2024-05-09 4:51 ` Christoph Hellwig
2024-05-09 2:20 ` Barry Song
2024-05-09 2:26 ` Barry Song
2024-05-09 2:30 ` Barry Song
2024-05-09 2:39 ` Gao Xiang [this message]
2024-05-09 3:09 ` Barry Song
2024-05-09 3:17 ` Gao Xiang
2024-05-09 3:11 ` Gao Xiang
2024-05-09 3:22 ` Hailong Liu
2024-05-09 3:33 ` Hailong Liu
2024-05-09 3:48 ` Barry Song
2024-05-09 4:19 ` Gao Xiang
2024-05-09 4:52 ` Christoph Hellwig
2024-05-09 6:12 ` Barry Song
2024-05-09 7:48 ` Michal Hocko
2024-05-09 8:06 ` Hailong Liu
2024-05-09 8:32 ` Barry Song
2024-05-09 8:57 ` Barry Song
2024-05-09 9:50 ` Hailong Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20d782ad-c059-4029-9c75-0ef278c98d81@linux.alibaba.com \
--to=hsiangkao@linux.alibaba.com \
--cc=21cnbao@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=chao@kernel.org \
--cc=hailong.liu@oppo.com \
--cc=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=liyangouwen1@oppo.com \
--cc=lstoakes@gmail.com \
--cc=urezki@gmail.com \
--cc=xiang@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).