Re: [PATCH tip/core/rcu 03/11] rcu/nocb: Provide separate no-CBs grace-period kthreads

From: Paul E. McKenney
Date: Sat Aug 03 2019 - 15:47:06 EST

Next message: Ajay Kaher: "[PATCH v6 1/3] [v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping"
Previous message: Souptick Joarder: "Re: [PATCH] arch/alpha: Remove dead code"
In reply to: Joel Fernandes: "Re: [PATCH tip/core/rcu 03/11] rcu/nocb: Provide separate no-CBs grace-period kthreads"
Next in thread: Joel Fernandes: "Re: [PATCH tip/core/rcu 03/11] rcu/nocb: Provide separate no-CBs grace-period kthreads"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, Aug 03, 2019 at 01:41:27PM -0400, Joel Fernandes wrote:
> On Thu, Aug 01, 2019 at 03:50:20PM -0700, Paul E. McKenney wrote:
> > Currently, there is one no-CBs rcuo kthread per CPU, and these kthreads
> > are divided into groups. The first rcuo kthread to come online in a
> > given group is that group's leader, and the leader both waits for grace
> > periods and invokes its CPU's callbacks. The non-leader rcuo kthreads
> > only invoke callbacks.
> >
> > This works well in the real-time/embedded environments for which it was
> > intended because such environments tend not to generate all that many
> > callbacks. However, given huge floods of callbacks, it is possible for
> > the leader kthread to be stuck invoking callbacks while its followers
> > wait helplessly while their callbacks pile up. This is a good recipe
> > for an OOM, and rcutorture's new callback-flood capability does generate
> > such OOMs.
> >
> > One strategy would be to wait until such OOMs start happening in
> > production, but similar OOMs have in fact happened starting in 2018.
> > It would therefore be wise to take a more proactive approach.
>
> I haven't looked much into nocbs/nohz_full stuff (yet). In particular, I did
> not even know that the rcuo threads do grace period life-cycle management and
> waiting, I thought only the RCU GP threads did :-/. however, it seems this is
> a completely separate grace-period management state machine outside of the
> RCU GP thread right?

No, the rcuo kthreads interact with the main RCU GP kthread, initiating
new grace periods when needed and being awakened as needed by the RCU
GP kthread.

> I was wondering for this patch, could we also just have the rcuo
> leader handle both callback execution and waking other non-leader threads at
> the same time? So like, execute few callbacks, then do the wake up of the
> non-leaders to execute their callbacks, the get back to executing their own
> callbacks, etc. That way we don't need a separate rcuog thread to wait for
> grace period, would that not work?

I did look into that, but it was more complex and also didn't foster
sharing of rcu_do_batch(), which used to only be for non-offloaded
callbacks but now does both. Besides which, invoking callbacks would
degrade the rcuog kthread's response to new callbacks and the like.

> If you don't mind could you share with me a kvm.sh command (which has config,
> boot parameters etc) that can produce the OOM without this patch? I'd
> like to take a closer look at it.

Here you go:

tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 120 --configs TREE04

If you add "--memory 1G" in mainline, the OOMs go away. Or at least
decrease substantially in probability.

> Is there also a short answer for my the RCU GP thread cannot do the job of
> these new rcuog threads?

First, the code is more complicated when you do it that way (and yes,
I did actually write it out in pen on paper). Second, if the CPU
corresponding to the combined grace-period/callback kthread is doing the
call_rcu() flooding, you are between a rock and a hard place. On the
one hand, you want that kthread to do nothing but invoke callbacks so
as to have half a chance of keeping up, and on the other hand you need
it to check state frequently so as to react in a timely fashion to a
CPU corresponding to one of its callback kthreads starting a second
callback flood.

Introducing rcug grace-period-only kthreads means that you get the best of
both worlds. Plus last year's flavor consolidation decreased the number
of rcuo kthreads from either 2N or 3N to N, so increasing it to only
(N + sqrt(N)) should be just fine. Though I would expect that there
will be at least some screaming and shouting. ;-)

Thanx, Paul

> thanks a lot,
>
> - Joel
>
>
> > This commit therefore features per-CPU rcuo kthreads that do nothing
> > but invoke callbacks. Instead of having one of these kthreads act as
> > leader, each group has a separate rcog kthread that handles grace periods
> > for its group. Because these rcuog kthreads do not invoke callbacks,
> > callback floods on one CPU no longer block callbacks from reaching the
> > rcuc callback-invocation kthreads on other CPUs.
> >
> > This change does introduce additional kthreads, however:
> >
> > 1. The number of additional kthreads is about the square root of
> > the number of CPUs, so that a 4096-CPU system would have only
> > about 64 additional kthreads. Note that recent changes
> > decreased the number of rcuo kthreads by a factor of two
> > (CONFIG_PREEMPT=n) or even three (CONFIG_PREEMPT=y), so
> > this still represents a significant improvement on most systems.
> >
> > 2. The leading "rcuo" of the rcuog kthreads should allow existing
> > scripting to affinity these additional kthreads as needed, the
> > same as for the rcuop and rcuos kthreads. (There are no longer
> > any rcuob kthreads.)
> >
> > 3. A state-machine approach was considered and rejected. Although
> > this would allow the rcuo kthreads to continue their dual
> > leader/follower roles, it complicates callback invocation
> > and makes it more difficult to consolidate rcuo callback
> > invocation with existing softirq callback invocation.
> >
> > The introduction of rcuog kthreads should thus be acceptable.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> > ---
> > kernel/rcu/tree.h | 6 +-
> > kernel/rcu/tree_plugin.h | 115 +++++++++++++++++++--------------------
> > 2 files changed, 61 insertions(+), 60 deletions(-)
> >
> > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > index 32b3348d3a4d..dc3c53cb9608 100644
> > --- a/kernel/rcu/tree.h
> > +++ b/kernel/rcu/tree.h
> > @@ -200,8 +200,8 @@ struct rcu_data {
> > atomic_long_t nocb_q_count_lazy; /* invocation (all stages). */
> > struct rcu_head *nocb_cb_head; /* CBs ready to invoke. */
> > struct rcu_head **nocb_cb_tail;
> > - struct swait_queue_head nocb_wq; /* For nocb kthreads to sleep on. */
> > - struct task_struct *nocb_cb_kthread;
> > + struct swait_queue_head nocb_cb_wq; /* For nocb kthreads to sleep on. */
> > + struct task_struct *nocb_gp_kthread;
> > raw_spinlock_t nocb_lock; /* Guard following pair of fields. */
> > int nocb_defer_wakeup; /* Defer wakeup of nocb_kthread. */
> > struct timer_list nocb_timer; /* Enforce finite deferral. */
> > @@ -211,6 +211,8 @@ struct rcu_data {
> > /* CBs waiting for GP. */
> > struct rcu_head **nocb_gp_tail;
> > bool nocb_gp_sleep; /* Is the nocb GP thread asleep? */
> > + struct swait_queue_head nocb_gp_wq; /* For nocb kthreads to sleep on. */
> > + struct task_struct *nocb_cb_kthread;
> > struct rcu_data *nocb_next_cb_rdp;
> > /* Next rcu_data in wakeup chain. */
> >
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 5a72700c3a32..c3b6493313ab 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -1531,7 +1531,7 @@ static void __wake_nocb_leader(struct rcu_data *rdp, bool force,
> > struct rcu_data *rdp_leader = rdp->nocb_gp_rdp;
> >
> > lockdep_assert_held(&rdp->nocb_lock);
> > - if (!READ_ONCE(rdp_leader->nocb_cb_kthread)) {
> > + if (!READ_ONCE(rdp_leader->nocb_gp_kthread)) {
> > raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
> > return;
> > }
> > @@ -1541,7 +1541,7 @@ static void __wake_nocb_leader(struct rcu_data *rdp, bool force,
> > del_timer(&rdp->nocb_timer);
> > raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
> > smp_mb(); /* ->nocb_gp_sleep before swake_up_one(). */
> > - swake_up_one(&rdp_leader->nocb_wq);
> > + swake_up_one(&rdp_leader->nocb_gp_wq);
> > } else {
> > raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
> > }
> > @@ -1646,7 +1646,7 @@ static void __call_rcu_nocb_enqueue(struct rcu_data *rdp,
> > smp_mb__after_atomic(); /* Store *old_rhpp before _wake test. */
> >
> > /* If we are not being polled and there is a kthread, awaken it ... */
> > - t = READ_ONCE(rdp->nocb_cb_kthread);
> > + t = READ_ONCE(rdp->nocb_gp_kthread);
> > if (rcu_nocb_poll || !t) {
> > trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
> > TPS("WakeNotPoll"));
> > @@ -1786,7 +1786,7 @@ static void rcu_nocb_wait_gp(struct rcu_data *rdp)
> > * No-CBs GP kthreads come here to wait for additional callbacks to show up.
> > * This function does not return until callbacks appear.
> > */
> > -static void nocb_leader_wait(struct rcu_data *my_rdp)
> > +static void nocb_gp_wait(struct rcu_data *my_rdp)
> > {
> > bool firsttime = true;
> > unsigned long flags;
> > @@ -1794,12 +1794,10 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
> > struct rcu_data *rdp;
> > struct rcu_head **tail;
> >
> > -wait_again:
> > -
> > /* Wait for callbacks to appear. */
> > if (!rcu_nocb_poll) {
> > trace_rcu_nocb_wake(rcu_state.name, my_rdp->cpu, TPS("Sleep"));
> > - swait_event_interruptible_exclusive(my_rdp->nocb_wq,
> > + swait_event_interruptible_exclusive(my_rdp->nocb_gp_wq,
> > !READ_ONCE(my_rdp->nocb_gp_sleep));
> > raw_spin_lock_irqsave(&my_rdp->nocb_lock, flags);
> > my_rdp->nocb_gp_sleep = true;
> > @@ -1838,7 +1836,7 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
> > trace_rcu_nocb_wake(rcu_state.name, my_rdp->cpu,
> > TPS("WokeEmpty"));
> > }
> > - goto wait_again;
> > + return;
> > }
> >
> > /* Wait for one grace period. */
> > @@ -1862,34 +1860,47 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
> > rdp->nocb_cb_tail = rdp->nocb_gp_tail;
> > *tail = rdp->nocb_gp_head;
> > raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
> > - if (rdp != my_rdp && tail == &rdp->nocb_cb_head) {
> > + if (tail == &rdp->nocb_cb_head) {
> > /* List was empty, so wake up the kthread. */
> > - swake_up_one(&rdp->nocb_wq);
> > + swake_up_one(&rdp->nocb_cb_wq);
> > }
> > }
> > +}
> >
> > - /* If we (the GP kthreads) don't have CBs, go wait some more. */
> > - if (!my_rdp->nocb_cb_head)
> > - goto wait_again;
> > +/*
> > + * No-CBs grace-period-wait kthread. There is one of these per group
> > + * of CPUs, but only once at least one CPU in that group has come online
> > + * at least once since boot. This kthread checks for newly posted
> > + * callbacks from any of the CPUs it is responsible for, waits for a
> > + * grace period, then awakens all of the rcu_nocb_cb_kthread() instances
> > + * that then have callback-invocation work to do.
> > + */
> > +static int rcu_nocb_gp_kthread(void *arg)
> > +{
> > + struct rcu_data *rdp = arg;
> > +
> > + for (;;)
> > + nocb_gp_wait(rdp);
> > + return 0;
> > }
> >
> > /*
> > * No-CBs CB kthreads come here to wait for additional callbacks to show up.
> > - * This function does not return until callbacks appear.
> > + * This function returns true ("keep waiting") until callbacks appear and
> > + * then false ("stop waiting") when callbacks finally do appear.
> > */
> > -static void nocb_follower_wait(struct rcu_data *rdp)
> > +static bool nocb_follower_wait(struct rcu_data *rdp)
> > {
> > - for (;;) {
> > - trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("FollowerSleep"));
> > - swait_event_interruptible_exclusive(rdp->nocb_wq,
> > - READ_ONCE(rdp->nocb_cb_head));
> > - if (smp_load_acquire(&rdp->nocb_cb_head)) {
> > - /* ^^^ Ensure CB invocation follows _head test. */
> > - return;
> > - }
> > - WARN_ON(signal_pending(current));
> > - trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WokeEmpty"));
> > + trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("FollowerSleep"));
> > + swait_event_interruptible_exclusive(rdp->nocb_cb_wq,
> > + READ_ONCE(rdp->nocb_cb_head));
> > + if (smp_load_acquire(&rdp->nocb_cb_head)) { /* VVV */
> > + /* ^^^ Ensure CB invocation follows _head test. */
> > + return false;
> > }
> > + WARN_ON(signal_pending(current));
> > + trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WokeEmpty"));
> > + return true;
> > }
> >
> > /*
> > @@ -1899,7 +1910,7 @@ static void nocb_follower_wait(struct rcu_data *rdp)
> > * have to do quite so many wakeups (as in they only need to wake the
> > * no-CBs GP kthreads, not the CB kthreads).
> > */
> > -static int rcu_nocb_kthread(void *arg)
> > +static int rcu_nocb_cb_kthread(void *arg)
> > {
> > int c, cl;
> > unsigned long flags;
> > @@ -1911,10 +1922,8 @@ static int rcu_nocb_kthread(void *arg)
> > /* Each pass through this loop invokes one batch of callbacks */
> > for (;;) {
> > /* Wait for callbacks. */
> > - if (rdp->nocb_gp_rdp == rdp)
> > - nocb_leader_wait(rdp);
> > - else
> > - nocb_follower_wait(rdp);
> > + while (nocb_follower_wait(rdp))
> > + continue;
> >
> > /* Pull the ready-to-invoke callbacks onto local list. */
> > raw_spin_lock_irqsave(&rdp->nocb_lock, flags);
> > @@ -2048,7 +2057,8 @@ void __init rcu_init_nohz(void)
> > static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp)
> > {
> > rdp->nocb_tail = &rdp->nocb_head;
> > - init_swait_queue_head(&rdp->nocb_wq);
> > + init_swait_queue_head(&rdp->nocb_cb_wq);
> > + init_swait_queue_head(&rdp->nocb_gp_wq);
> > rdp->nocb_cb_tail = &rdp->nocb_cb_head;
> > raw_spin_lock_init(&rdp->nocb_lock);
> > timer_setup(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, 0);
> > @@ -2056,50 +2066,39 @@ static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp)
> >
> > /*
> > * If the specified CPU is a no-CBs CPU that does not already have its
> > - * rcuo kthread, spawn it. If the CPUs are brought online out of order,
> > - * this can require re-organizing the GP-CB relationships.
> > + * rcuo CB kthread, spawn it. Additionally, if the rcuo GP kthread
> > + * for this CPU's group has not yet been created, spawn it as well.
> > */
> > static void rcu_spawn_one_nocb_kthread(int cpu)
> > {
> > - struct rcu_data *rdp;
> > - struct rcu_data *rdp_last;
> > - struct rcu_data *rdp_old_leader;
> > - struct rcu_data *rdp_spawn = per_cpu_ptr(&rcu_data, cpu);
> > + struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > + struct rcu_data *rdp_gp;
> > struct task_struct *t;
> >
> > /*
> > * If this isn't a no-CBs CPU or if it already has an rcuo kthread,
> > * then nothing to do.
> > */
> > - if (!rcu_is_nocb_cpu(cpu) || rdp_spawn->nocb_cb_kthread)
> > + if (!rcu_is_nocb_cpu(cpu) || rdp->nocb_cb_kthread)
> > return;
> >
> > /* If we didn't spawn the GP kthread first, reorganize! */
> > - rdp_old_leader = rdp_spawn->nocb_gp_rdp;
> > - if (rdp_old_leader != rdp_spawn && !rdp_old_leader->nocb_cb_kthread) {
> > - rdp_last = NULL;
> > - rdp = rdp_old_leader;
> > - do {
> > - rdp->nocb_gp_rdp = rdp_spawn;
> > - if (rdp_last && rdp != rdp_spawn)
> > - rdp_last->nocb_next_cb_rdp = rdp;
> > - if (rdp == rdp_spawn) {
> > - rdp = rdp->nocb_next_cb_rdp;
> > - } else {
> > - rdp_last = rdp;
> > - rdp = rdp->nocb_next_cb_rdp;
> > - rdp_last->nocb_next_cb_rdp = NULL;
> > - }
> > - } while (rdp);
> > - rdp_spawn->nocb_next_cb_rdp = rdp_old_leader;
> > + rdp_gp = rdp->nocb_gp_rdp;
> > + if (!rdp_gp->nocb_gp_kthread) {
> > + t = kthread_run(rcu_nocb_gp_kthread, rdp_gp,
> > + "rcuog/%d", rdp_gp->cpu);
> > + if (WARN_ONCE(IS_ERR(t), "%s: Could not start rcuo GP kthread, OOM is now expected behavior\n", __func__))
> > + return;
> > + WRITE_ONCE(rdp_gp->nocb_gp_kthread, t);
> > }
> >
> > /* Spawn the kthread for this CPU. */
> > - t = kthread_run(rcu_nocb_kthread, rdp_spawn,
> > + t = kthread_run(rcu_nocb_cb_kthread, rdp,
> > "rcuo%c/%d", rcu_state.abbr, cpu);
> > - if (WARN_ONCE(IS_ERR(t), "%s: Could not start rcuo kthread, OOM is now expected behavior\n", __func__))
> > + if (WARN_ONCE(IS_ERR(t), "%s: Could not start rcuo CB kthread, OOM is now expected behavior\n", __func__))
> > return;
> > - WRITE_ONCE(rdp_spawn->nocb_cb_kthread, t);
> > + WRITE_ONCE(rdp->nocb_cb_kthread, t);
> > + WRITE_ONCE(rdp->nocb_gp_kthread, rdp_gp->nocb_gp_kthread);
> > }
> >
> > /*
> > --
> > 2.17.1
> >
>

Next message: Ajay Kaher: "[PATCH v6 1/3] [v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping"
Previous message: Souptick Joarder: "Re: [PATCH] arch/alpha: Remove dead code"
In reply to: Joel Fernandes: "Re: [PATCH tip/core/rcu 03/11] rcu/nocb: Provide separate no-CBs grace-period kthreads"
Next in thread: Joel Fernandes: "Re: [PATCH tip/core/rcu 03/11] rcu/nocb: Provide separate no-CBs grace-period kthreads"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]