Received-SPF: pass (google.com: domain of linux-kernel+bounces-174078-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240508125808.28882-1-hailong.liu@oppo.com> <CAGsJ_4xN0MBz_73wUvMp74upd9SaQ+TCRJufEj26Y619Rtr7Zw@mail.gmail.com>
 <20d782ad-c059-4029-9c75-0ef278c98d81@linux.alibaba.com>
In-Reply-To: <20d782ad-c059-4029-9c75-0ef278c98d81@linux.alibaba.com>
From: Barry Song <21cnbao@gmail.com>
Date: Thu, 9 May 2024 15:09:07 +1200
Message-ID: <CAGsJ_4xoqdd7+vWAnAdaib_NM8Snf=pxkThT1-b0aU-CPaQBYA@mail.gmail.com>
Subject: Re: [RFC PATCH] mm/vmalloc: fix vmalloc which may return null if
 called with __GFP_NOFAIL
To: Gao Xiang <hsiangkao@linux.alibaba.com>
Cc: hailong.liu@oppo.com, 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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 9, 2024 at 2:39=E2=80=AFPM Gao Xiang <hsiangkao@linux.alibaba.c=
om> wrote:
>
> Hi,
>
> On 2024/5/9 10:20, Barry Song wrote:
> > On Thu, May 9, 2024 at 12:58=E2=80=AFAM <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_page=
s()
> >> 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(curr=
ent))
> >>                          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: vm=
alloc
> >      does not guarantee successful allocation, it has never been called=
 with
> >      __GFP_NOFAIL and threfore either should not be used for any rollba=
cks or
> >      should handle such errors correctly and not lead to critical failu=
res.
> > "
> >
> > 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.

I completely understand that you're not concerned about the origin of
the memory,
such as whether it's organized by all zero-order pages. However, in the eve=
nt
that someone else allocates a large memory, like several megabytes with the
NOFAIL flag, commit dd544141b9eb aims to halt the allocation before success
if the process being allocated is targeted for termination of OOM-killer.

With the current patch, we miss the opportunity for early allocation
termination.
However, if the size of the kvmalloc() is small, as in your case, I
believe it should
be perfectly fine. but do we have any way to prevent large size allocation =
with
NOFAIL?

>
>
> Thanks,
> Gao XIang

Thanks
Barry