> On Sep 7, 2021, at 11:41 AM, Mel Gorman <mgorman@xxxxxxxx> wrote: > > On Tue, Sep 07, 2021 at 02:53:48PM +0000, Chuck Lever III wrote: >>> So maybe we really don't want reclaim_progress_wait(), and all current >>> callers of congestion_wait() which are just waiting for allocation to >>> succeed should be either change to use __GFP_NOFAIL, or to handle >>> failure. >> >> I had completely forgotten about GFP_NOFAIL. That seems like the >> preferred answer, as it avoids an arbitrary time-based wait for >> a memory resource. (And maybe svc_rqst_alloc() should also get >> the NOFAIL treatment?). >> > > Don't use NOFAIL. It's intended for allocation requests that if they fail, > there is no sane way for the kernel to recover. Amoung other things, > it can access emergency memory reserves and if not returned quickly may > cause premature OOM or even a livelock. > >> The question in my mind is how the new alloc_pages_bulk() will >> behave when passed NOFAIL. I would expect that NOFAIL would simply >> guarantee that at least one page is allocated on every call. >> > > alloc_pages_bulk ignores GFP_NOFAIL. If called repeatedly, it might pass > the GFP_NOFAIL flag to allocate at least one page but you'll be depleting > reserves to do it from a call site that has other options for recovery. True, an allocation failure in svc_alloc_arg() won't cause kernel instability. But AFAICS svc_alloc_arg() can't do anything but call again if an allocation request fails to make forward progress. There really aren't other options for recovery. On it's face, svc_alloc_arg() seems to match the "use the flag rather than opencode [an] endless loop around [the] allocator" comment below. > The docs say it better > > * %__GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller > * cannot handle allocation failures. The allocation could block > * indefinitely but will never return with failure. Testing for > * failure is pointless. > * New users should be evaluated carefully (and the flag should be > * used only when there is no reasonable failure policy) but it is > * definitely preferable to use the flag rather than opencode endless > * loop around allocator. As an aside, there's nothing here that suggests that using NOFAIL would be risky. And the "no reasonable failure policy" phrasing is a parenthetical, but it seems to be most pertinent. IMHO the comment should be updated to match the current implementation of NOFAIL. > * Using this flag for costly allocations is _highly_ discouraged I don't have a preference about using NOFAIL. I think it's a good idea to base the wait for resources on the availability of those resources rather than an arbitrary time interval, so I'd like to see the schedule_timeout() in svc_alloc_arg() replaced with something smarter. -- Chuck Lever
