Re: [PATCH 1/2] mm: clarify __GFP_MEMALLOC usage

[John Hubbard]

On 4/6/20 6:00 PM, NeilBrown wrote:
...
> > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > > index e5b817cb86e7..9cacef1a3ee0 100644
> > > --- a/include/linux/gfp.h
> > > +++ b/include/linux/gfp.h
> > > @@ -110,6 +110,11 @@ struct vm_area_struct;
> > >     * the caller guarantees the allocation will allow more memory to be freed
> > >     * very shortly e.g. process exiting or swapping. Users either should
> > >     * be the MM or co-ordinating closely with the VM (e.g. swap over NFS).
> > > + * Users of this flag have to be extremely careful to not deplete the reserve
> > > + * completely and implement a throttling mechanism which controls the consumption
> > > + * of the reserve based on the amount of freed memory.
> > > + * Usage of a pre-allocated pool (e.g. mempool) should be always considered before
> > > + * using this flag.
>
> I think this version is pretty good.
>
> > >     *
> > >     * %__GFP_NOMEMALLOC is used to explicitly forbid access to emergency reserves.
> > >     * This takes precedence over the %__GFP_MEMALLOC flag if both are set.
> >
> > Hi Michal and all,
> >
> > How about using approximately this wording instead? I found Neil's wording to be
> > especially helpful so I mixed it in. (Also fixed a couple of slight 80-col overruns.)
> >
> > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > index be2754841369..c247a911d8c7 100644
> > --- a/include/linux/gfp.h
> > +++ b/include/linux/gfp.h
> > @@ -111,6 +111,15 @@ struct vm_area_struct;
> >     * very shortly e.g. process exiting or swapping. Users either should
> >     * be the MM or co-ordinating closely with the VM (e.g. swap over NFS).
> >     *
> > + * To be extra clear: users of __GFP_MEMALLOC must be working to free other
> > + * memory, and that other memory needs to be freed "soon"; specifically, before
> > + * the reserve is exhausted. This generally implies a throttling mechanism that
> > + * balances the amount of __GFP_MEMALLOC memory used against the amount that the
> > + * caller is about to free.
>
> I don't like this change. "balances the amount ... is about to free"
> does say anything about time, so it doesn't seem to be about throttling.
>
> I think it is hard to write rules because the rules are a bit spongey.
>
> With mempools, we have a nice clear rule.  When you allocate from a
> mempool you must have a clear path to freeing that allocation which will
> not block on memory allocation except from a subordinate mempool.  This
> implies a partial ordering between mempools.  When you have layered
> block devices the path through the layers from filesystem down to
> hardware defines the order.  It isn't enforced, but it is quite easy to
> reason about.
>
> GFP_MEMALLOC effectively provides multiple mempools.  So it could
> theoretically deadlock if multiple long dependency chains
> happened. i.e. if 1000 threads each make a GFP_MEMALLOC allocation and
> then need to make another one before the first can be freed - then you
> hit problems.  There is no formal way to guarantee that this doesn't
> happen.  We just say "be gentle" and minimize the users of this flag,
> and keep more memory in reserve than we really need.
> Note that 'threads' here might not be Linux tasks.  If you have an IO
> request that proceed asynchronously, moving from queue to queue and
> being handled by different task, then each one is a "thread" for the
> purpose of understanding mem-alloc dependency.
>
> So maybe what I really should focus on is not how quickly things happen,
> but how many happen concurrently.  The idea of throttling is to allow
> previous requests to complete before we start too many more.
>
> With Swap-over-NFS, some of the things that might need to be allocated
> are routing table entries.  These scale with the number of NFS servers
> rather than the number of IO requests, so they are not going to cause
> concurrency problems.
> We also need memory to store replies, but these never exceed the number
> of pending requests, so there is limited concurrency there.
> NFS can send a lot of requests in parallel, but the main limit is the
> RPC "slot table" and while that grows dynamically, it does so with
> GFP_NOFS, so it can block or fail (I wonder if that should explicitly
> disable the use of the reserves).
>
> So there a limit on concurrency imposed by non-GFP_MEMALLOC allocations
>
> So ... maybe the documentation should say that boundless concurrency of
> allocations (i.e. one module allocating a boundless number of times
> before previous allocations are freed) must be avoided.

Well, that's a good discussion that you just wrote, above, and I think it
demonstrates that it's hard to describe the situation in just a couple of
sentences. With that in mind, perhaps it's best to take the above notes
as a starting point, adjust them slightly and drop them into
Documentation/core-api/memory-allocation.rst ?

Then the comments here could refer to it.


thanks,
--
John Hubbard
NVIDIA