Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 1 Apr 2020 09:23:59 +0200
From:   Michal Hocko <mhocko@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        rcu@vger.kernel.org, willy@infradead.org, peterz@infradead.org,
        neilb@suse.com, vbabka@suse.cz, mgorman@suse.de,
        Andrew Morton <akpm@linux-foundation.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH RFC] rcu/tree: Use GFP_MEMALLOC for alloc memory to free
 memory pattern
Message-ID: <20200401072359.GC22681@dhcp22.suse.cz>
References: <20200331131628.153118-1-joel@joelfernandes.org>
 <20200331145806.GB236678@google.com>
 <20200331153450.GM30449@dhcp22.suse.cz>
 <20200331160117.GA170994@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200331160117.GA170994@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue 31-03-20 12:01:17, Joel Fernandes wrote:
> On Tue, Mar 31, 2020 at 05:34:50PM +0200, Michal Hocko wrote:
> > On Tue 31-03-20 10:58:06, Joel Fernandes wrote:
> > [...]
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 4be763355c9fb..965deefffdd58 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3149,7 +3149,7 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
> > > >  
> > > >  	if (!ptr)
> > > >  		ptr = kmalloc(sizeof(unsigned long *) +
> > > > -				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
> > > > +				sizeof(struct rcu_head), GFP_MEMALLOC);
> > > 
> > > Just to add, the main requirements here are:
> > > 1. Allocation should be bounded in time.
> > > 2. Allocation should try hard (possibly tapping into reserves)
> > > 3. Sleeping is Ok but should not affect the time bound.
> > 
> > 
> > __GFP_ATOMIC | __GFP_HIGH is the way to get an additional access to
> > memory reserves regarless of the sleeping status.
> > 
> > Using __GFP_MEMALLOC is quite dangerous because it can deplete _all_ the
> > memory. What does prevent the above code path to do that?

Neil has provided a nice explanation down the email thread. But let me
clarify few things here.

> Can you suggest what prevents other users of GFP_MEMALLOC from doing that
> also? 

There is no explicit mechanism which is indeed unfortunate. The only
user real user of the flag is Swap over NFS AFAIK. I have never dared to
look into details on how the complete reserves depletion is prevented.
Mel would be much better fit here.

> That's the whole point of having a reserve, in normal usage no one will
> use it, but some times you need to use it. Keep in mind this is not a common
> case in this code here, this is triggered only if earlier allocation attempts
> failed. Only *then* we try with GFP_MEMALLOC with promises to free additional
> memory soon.

You are right that this is the usecase for the flag. But this should be
done with an extreme care because the core MM relies on those reserves
so any other users should better make sure they do not consume a lot
from reserves as well. 

> > If a partial access to reserves is sufficient then why the existing
> > modifiers (mentioned above are not sufficient?
> 
> The point with using GFP_MEMALLOC is it is useful for situations where you
> are about to free memory and needed some memory temporarily, to free that. It
> depletes it a bit temporarily to free even more. Is that not the point of
> PF_MEMALLOC?
> * %__GFP_MEMALLOC allows access to all memory. This should only be used when
>  * 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).
> 
> I was just recommending usage of this flag here because it fits the
> requirement of allocating some memory to free some memory. I am also Ok with
> GFP_ATOMIC with the GFP_NOWARN removed, if you are Ok with that.

Maybe we need to refine this documentation to be more explicit about an
extreme care to be taken when using the flag.

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index e5b817cb86e7..e436a7e28392 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -110,6 +110,9 @@ 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
+ * based on the amount of freed memory.
  *
  * %__GFP_NOMEMALLOC is used to explicitly forbid access to emergency reserves.
  * This takes precedence over the %__GFP_MEMALLOC flag if both are set.
-- 
Michal Hocko
SUSE Labs