Re: [PATCH] SUNRPC: use congestion_wait() in svc_alloc_args()

[Chuck Lever III <chuck.lever@oracle.com>]

> On Sep 7, 2021, at 11:41 AM, Mel Gorman <mgorman@suse.com> 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