Re: [PATCH v2 2/4] mm/vmalloc: add support for __GFP_NOFAIL

[Michal Hocko]

On Tue 23-11-21 17:02:38, 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?

This is what xfs is likely going to do if we do not provide the
functionality. I just do not see why that would be a better outcome
though. My longterm experience tells me that whenever we ignore
requirements by other subsystems then those requirements materialize in
some form in the end. In many cases done either suboptimaly or outright
wrong. This might be not the case for xfs as the quality of
implementation is high there but this is not the case in general.

Even if people start using vmalloc(GFP_NOFAIL) out of lazyness or for
any other stupid reason then what? Is that something we should worry
about? Retrying within the allocator doesn't make the things worse. In
fact it is just easier to find such abusers by grep which would be more
elaborate with custom retry loops.
 
[...]
> > > +		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.

vmalloc has two potential failure modes. Depleted memory and vmalloc
space. So there are two different events to wait for. I do agree that
schedule_timeout_uninterruptible is both ugly and very simple but do we
really need a much more sophisticated solution at this stage?
-- 
Michal Hocko
SUSE Labs