lkml.org 
[lkml]   [2021]   [Nov]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/4] mm/vmalloc: add support for __GFP_NOFAIL
    On Wed 24-11-21 21:11:42, Uladzislau Rezki wrote:
    > On Tue, Nov 23, 2021 at 05:02:38PM -0800, Andrew Morton wrote:
    > > On Tue, 23 Nov 2021 20:01:50 +0100 Uladzislau Rezki <urezki@gmail.com> wrote:
    > >
    > > > On Mon, Nov 22, 2021 at 04:32:31PM +0100, Michal Hocko wrote:
    > > > > From: Michal Hocko <mhocko@suse.com>
    > > > >
    > > > > Dave Chinner has mentioned that some of the xfs code would benefit from
    > > > > kvmalloc support for __GFP_NOFAIL because they have allocations that
    > > > > cannot fail and they do not fit into a single page.
    > >
    > > Perhaps we should tell xfs "no, do it internally". Because this is a
    > > rather nasty-looking thing - do we want to encourage other callsites to
    > > start using it?
    > >
    > > > > The large part of the vmalloc implementation already complies with the
    > > > > given gfp flags so there is no work for those to be done. The area
    > > > > and page table allocations are an exception to that. Implement a retry
    > > > > loop for those.
    > > > >
    > > > > Add a short sleep before retrying. 1 jiffy is a completely random
    > > > > timeout. Ideally the retry would wait for an explicit event - e.g.
    > > > > a change to the vmalloc space change if the failure was caused by
    > > > > the space fragmentation or depletion. But there are multiple different
    > > > > reasons to retry and this could become much more complex. Keep the retry
    > > > > simple for now and just sleep to prevent from hogging CPUs.
    > > > >
    > >
    > > Yes, the horse has already bolted. But we didn't want that horse anyway ;)
    > >
    > > I added GFP_NOFAIL back in the mesozoic era because quite a lot of
    > > sites were doing open-coded try-forever loops. I thought "hey, they
    > > shouldn't be doing that in the first place, but let's at least
    > > centralize the concept to reduce code size, code duplication and so
    > > it's something we can now grep for". But longer term, all GFP_NOFAIL
    > > sites should be reworked to no longer need to do the retry-forever
    > > thing. In retrospect, this bright idea of mine seems to have added
    > > license for more sites to use retry-forever. Sigh.
    > >
    > > > > + if (nofail) {
    > > > > + schedule_timeout_uninterruptible(1);
    > > > > + goto again;
    > > > > + }
    > >
    > > The idea behind congestion_wait() is to prevent us from having to
    > > hard-wire delays like this. congestion_wait(1) would sleep for up to
    > > one millisecond, but will return earlier if reclaim events happened
    > > which make it likely that the caller can now proceed with the
    > > allocation event, successfully.
    > >
    > > However it turns out that congestion_wait() was quietly broken at the
    > > block level some time ago. We could perhaps resurrect the concept at
    > > another level - say by releasing congestion_wait() callers if an amount
    > > of memory newly becomes allocatable. This obviously asks for inclusion
    > > of zone/node/etc info from the congestion_wait() caller. But that's
    > > just an optimization - if the newly-available memory isn't useful to
    > > the congestion_wait() caller, they just fail the allocation attempts
    > > and wait again.
    > >
    > > > well that is sad...
    > > > I have raised two concerns in our previous discussion about this change,
    > >
    > > Can you please reiterate those concerns here?
    > >
    > 1. I proposed to repeat(if fails) in one solid place, i.e. get rid of
    > duplication and spreading the logic across several places. This is about
    > simplification.

    I am all for simplifications. But the presented simplification lead to 2) and ...

    > 2. Second one is about to do an unwinding and release everything what we
    > have just accumulated in terms of memory consumption. The failure might
    > occur, if so a condition we are in is a low memory one or high memory
    > pressure. In this case, since we are about to sleep some milliseconds
    > in order to repeat later, IMHO it makes sense to release memory:
    >
    > - to prevent killing apps or possible OOM;
    > - we can end up looping quite a lot of time or even forever if users do
    > nasty things with vmalloc API and __GFP_NOFAIL flag.

    ... this is where we disagree and I have tried to explain why. The primary
    memory to allocate are pages to back the vmalloc area. Failing to
    allocate few page tables - which btw. do not fail as they are order-0 -
    and result into the whole and much more expensive work to allocate the
    former is really wasteful. You've had a concern about OOM killer
    invocation while retrying the page table allocation but you should
    realize that page table allocations might already invoke OOM killer so that
    is absolutely nothing new.
    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2021-11-25 09:49    [W:4.152 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site