From: Joel Fernandes <joel@joelfernandes.org>
To: NeilBrown <neilb@suse.de>
Cc: Michal Hocko <mhocko@kernel.org>,
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
Date: Tue, 31 Mar 2020 23:25:55 -0400 [thread overview]
Message-ID: <20200401032555.GA175966@google.com> (raw)
In-Reply-To: <877dz0yxoa.fsf@notabene.neil.brown.name>
On Wed, Apr 01, 2020 at 09:19:49AM +1100, NeilBrown wrote:
> On Tue, Mar 31 2020, 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?
> >
> > Can you suggest what prevents other users of GFP_MEMALLOC from doing that
> > also? 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.
>
> I think that "soon" is the key point. Users of __GFP_MEMALLOC certainly
> must be working to free other memory, that other memory needs to be freed
> "soon". In particular - sooner than all the reserve is exhausted. This
> can require rate-limiting. If one allocation can result in one page
> being freed, that is good and it is probably OK to have 1000 allocations
> resulting in 1000 pages being freed soon. But 10 million allocation to
> gain 10 million pages is not such a good thing and shouldn't be needed.
> Once those first 1000 pages have been freed, you won't need
> __GFP_MEMALLOC allocations any more, and you must be prepare to wait for
> them.
>
> So where does the rate-limiting happen in your proposal? A GP can be
> multiple milliseconds, which is time for lots of memory to be allocated
> and for rcu-free queues to grow quite large.
>
> You mention a possible fall-back of calling synchronize_rcu(). I think
> that needs to be a fallback that happens well before __GFP_MEMALLOC is
> exhausted. You need to choose some maximum amount that you will
> allocate, then use synchronize_rcu() (or probably the _expedited
> version) after that. The pool of reserves are certainly there for you
> to use, but not for you to exhaust.
>
> If you have your own rate-limiting, then I think __GFP_MEMALLOC is
> probably OK, and also you *don't* want the memalloc to wait. If memory
> cannot be allocated immediately, you need to use your own fallback.
Thanks a lot for explaining in detail, the RFC patch has served its purpose
well ;-)
On discussing with RCU comrades, we agreed to not use GFP_MEMALLOC. But
instead pre-allocate a cache (we do have a cache but it is not yet
pre-allocated, just allocated on demand).
About the rate limiting, we would fallback to synchronize_rcu() instead of
sleeping in case of trobule. However I would like to add a warning if we ever
hit the troublesome path mainly because that means we depleted the
pre-allocated cache and perhaps the user should switch to adding an rcu_head
in their structure to reduce latency. I'm adding that warning to my tree:
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 4be763355c9fb..6172e6296dd7d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -110,6 +110,10 @@ module_param(rcu_fanout_exact, bool, 0444);
static int rcu_fanout_leaf = RCU_FANOUT_LEAF;
module_param(rcu_fanout_leaf, int, 0444);
int rcu_num_lvls __read_mostly = RCU_NUM_LVLS;
+/* Silence the kvfree_rcu() complaint (warning) that it blocks */
+int rcu_kfree_nowarn;
+module_param(rcu_kfree_nowarn, int, 0444);
+
/* Number of rcu_nodes at specified level. */
int num_rcu_lvl[] = NUM_RCU_LVL_INIT;
int rcu_num_nodes __read_mostly = NUM_RCU_NODES; /* Total # rcu_nodes in use. */
@@ -3266,6 +3270,12 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
* state.
*/
if (!success) {
+ /*
+ * Please embed an rcu_head and pass it along if you hit this
+ * warning. Doing so would avoid long kfree_rcu() latencies.
+ */
+ if (!rcu_kfree_nowarn)
+ WARN_ON_ONCE(1);
debug_rcu_head_unqueue(ptr);
synchronize_rcu();
kvfree(ptr);
next prev parent reply other threads:[~2020-04-01 3:26 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-31 13:16 [PATCH RFC] rcu/tree: Use GFP_MEMALLOC for alloc memory to free memory pattern Joel Fernandes (Google)
2020-03-31 14:04 ` Uladzislau Rezki
2020-03-31 15:09 ` Joel Fernandes
2020-03-31 16:01 ` Uladzislau Rezki
2020-03-31 17:02 ` Uladzislau Rezki
2020-03-31 17:49 ` Paul E. McKenney
2020-03-31 18:30 ` Joel Fernandes
2020-04-01 12:25 ` Uladzislau Rezki
2020-04-01 13:47 ` Paul E. McKenney
2020-04-01 18:16 ` Uladzislau Rezki
2020-04-01 18:26 ` Paul E. McKenney
2020-04-01 18:37 ` Uladzislau Rezki
2020-04-01 18:54 ` Paul E. McKenney
2020-04-01 19:05 ` Uladzislau Rezki
2020-04-01 19:34 ` Paul E. McKenney
2020-04-01 19:35 ` Uladzislau Rezki
2020-03-31 14:58 ` Joel Fernandes
2020-03-31 15:34 ` Michal Hocko
2020-03-31 16:01 ` Joel Fernandes
2020-03-31 22:19 ` NeilBrown
2020-04-01 3:25 ` Joel Fernandes [this message]
2020-04-01 4:52 ` NeilBrown
2020-04-01 7:23 ` Michal Hocko
2020-04-01 11:14 ` joel
2020-04-01 12:05 ` Michal Hocko
2020-04-01 13:14 ` Mel Gorman
2020-04-01 14:45 ` Joel Fernandes
2020-03-31 16:12 ` Uladzislau Rezki
2020-04-01 7:09 ` Michal Hocko
2020-04-01 12:32 ` Uladzislau Rezki
2020-04-01 12:55 ` Michal Hocko
2020-04-01 13:08 ` Uladzislau Rezki
2020-04-01 13:15 ` Michal Hocko
2020-04-01 13:22 ` Uladzislau Rezki
2020-04-01 15:28 ` Michal Hocko
2020-04-01 15:46 ` Uladzislau Rezki
2020-04-01 15:57 ` Paul E. McKenney
2020-04-01 16:10 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200401032555.GA175966@google.com \
--to=joel@joelfernandes.org \
--cc=akpm@linux-foundation.org \
--cc=jiangshanlai@gmail.com \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mgorman@suse.de \
--cc=mhocko@kernel.org \
--cc=neilb@suse.com \
--cc=neilb@suse.de \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=vbabka@suse.cz \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).