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@xxxxxxxxx> wrote:
> >
> > > On Mon, Nov 22, 2021 at 04:32:31PM +0100, Michal Hocko wrote:
> > > > From: Michal Hocko <mhocko@xxxxxxxx>
> > > >
> > > > 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
