| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20190321135209.GT4102@linux.ibm.com>
Date: Thu, 21 Mar 2019 06:52:09 -0700
From: "Paul E. McKenney" <paulmck@...ux.ibm.com>
To: Joel Fernandes <joel@...lfernandes.org>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
linux-kernel@...r.kernel.org,
Josh Triplett <josh@...htriplett.org>,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Lai Jiangshan <jiangshanlai@...il.com>, tglx@...utronix.de,
Mike Galbraith <bitbucket@...ine.de>, rcu@...r.kernel.org
Subject: Re: [PATCH] rcu: Allow to eliminate softirq processing from rcutree
On Thu, Mar 21, 2019 at 08:06:56AM -0400, Joel Fernandes wrote:
> On Wed, Mar 20, 2019 at 12:28:35PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-03-19 20:26:13 [-0400], Joel Fernandes wrote:
> > > > @@ -2769,19 +2782,121 @@ static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > > > {
> > > > if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > > > return;
> > > > - if (likely(!rcu_state.boost)) {
> > > > - rcu_do_batch(rdp);
> > > > - return;
> > > > - }
> > > > - invoke_rcu_callbacks_kthread();
> > > > + rcu_do_batch(rdp);
> > >
> > > Looks like a nice change, but one question...
> > >
> > > Consider the case where rcunosoftirq boot option is not passed.
> > >
> > > Before, if RCU_BOOST=y, then callbacks would be invoked in rcuc threads if
> > > possible, by those threads being woken up from within the softirq context
> > > (in invoke_rcu_callbacks).
> > >
> > > Now, if RCU_BOOST=y, then callbacks would only be invoked in softirq context
> > > and not in the threads at all. Because rcu_softirq_enabled = false, so the
> > > path executes:
> > > rcu_read_unlock_special() ->
> > > raise_softirq_irqsoff() ->
> > > rcu_process_callbacks_si() ->
> > > rcu_process_callbacks() ->
> > > invoke_rcu_callbacks() ->
> > > rcu_do_batch()
> > >
> > > This seems like a behavioral change to me. This makes the callbacks always
> > > execute from the softirq context and not the threads when boosting is
> > > configured. IMO in the very least, such behavioral change should be
> > > documented in the change.
> > >
> > > One way to fix this I think could be, if boosting is enabled, then set
> > > rcu_softirq_enabled to false by default so the callbacks are still executed
> > > in the rcuc threads.
> > >
> > > Did I miss something? Sorry if I did, thanks!
> >
> > So with all the swaps and reorder we talking about this change:
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 0a719f726e149..82810483bfc6c 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2306,20 +2306,6 @@ static void rcu_core_si(struct softirq_action *h)
> > rcu_core();
> > }
> >
> > -/*
> > - * Schedule RCU callback invocation. If the running implementation of RCU
> > - * does not support RCU priority boosting, just do a direct call, otherwise
> > - * wake up the per-CPU kernel kthread. Note that because we are running
> > - * on the current CPU with softirqs disabled, the rcu_cpu_kthread_task
> > - * cannot disappear out from under us.
> > - */
> > -static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > -{
> > - if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > - return;
> > - rcu_do_batch(rdp);
> > -}
> > -
> > static void rcu_wake_cond(struct task_struct *t, int status)
> > {
> > /*
> > @@ -2330,6 +2316,19 @@ static void rcu_wake_cond(struct task_struct *t, int status)
> > wake_up_process(t);
> > }
> >
> > +static void invoke_rcu_core_kthread(void)
> > +{
> > + struct task_struct *t;
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + __this_cpu_write(rcu_data.rcu_cpu_has_work, 1);
> > + t = __this_cpu_read(rcu_data.rcu_cpu_kthread_task);
> > + if (t != NULL && t != current)
> > + rcu_wake_cond(t, __this_cpu_read(rcu_data.rcu_cpu_kthread_status));
> > + local_irq_restore(flags);
> > +}
> > +
> > static bool rcu_softirq_enabled = true;
> >
> > static int __init rcunosoftirq_setup(char *str)
> > @@ -2339,26 +2338,33 @@ static int __init rcunosoftirq_setup(char *str)
> > }
> > __setup("rcunosoftirq", rcunosoftirq_setup);
> >
> > +/*
> > + * Schedule RCU callback invocation. If the running implementation of RCU
> > + * does not support RCU priority boosting, just do a direct call, otherwise
> > + * wake up the per-CPU kernel kthread. Note that because we are running
> > + * on the current CPU with softirqs disabled, the rcu_cpu_kthread_task
> > + * cannot disappear out from under us.
> > + */
> > +static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > +{
> > + if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > + return;
> > + if (rcu_state.boost || rcu_softirq_enabled)
> > + invoke_rcu_core_kthread();
>
> Here shouldn't it be this?
> if (rcu_state.boost || !rcu_softirq_enabled)
>
> Also the rcu/dev branch has the following hunk where we unconditionally
> invoke rcu_do_batch even when boosting which would still have the issue I
> pointed. I would suggest Sebastian to post the latest v4 or v5 with all diff
> squashed, and then we do another round of review with latest patch, thanks!
I believe that -rcu has this change. But it looks like there still are
failures, so yes, further review is necessary and deeply appreciated!
Thanx, Paul
> @@ -2306,18 +2320,110 @@ static void invoke_rcu_callbacks(struct rcu_data *rdp)
> {
> if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> return;
> - if (likely(!rcu_state.boost)) {
> - rcu_do_batch(rdp);
> - return;
> - }
> - invoke_rcu_callbacks_kthread();
> + rcu_do_batch(rdp);
> +}
> +
>
> thanks,
>
> - Joel
>
>
> > + rcu_do_batch(rdp);
> > +}
> > +
> > /*
> > * Wake up this CPU's rcuc kthread to do RCU core processing.
> > */
> > static void invoke_rcu_core(void)
> > {
> > - unsigned long flags;
> > - struct task_struct *t;
> > -
> > if (!cpu_online(smp_processor_id()))
> > return;
> > - if (rcu_softirq_enabled) {
> > + if (rcu_softirq_enabled)
> > raise_softirq(RCU_SOFTIRQ);
> > - } else {
> > - local_irq_save(flags);
> > - __this_cpu_write(rcu_data.rcu_cpu_has_work, 1);
> > - t = __this_cpu_read(rcu_data.rcu_cpu_kthread_task);
> > - if (t != NULL && t != current)
> > - rcu_wake_cond(t, __this_cpu_read(rcu_data.rcu_cpu_kthread_status));
> > - local_irq_restore(flags);
> > - }
> > + else
> > + invoke_rcu_core_kthread();
> > }
> >
> > static void rcu_cpu_kthread_park(unsigned int cpu)
> > @@ -2426,7 +2432,8 @@ static int __init rcu_spawn_core_kthreads(void)
> > per_cpu(rcu_data.rcu_cpu_has_work, cpu) = 0;
> > if (!IS_ENABLED(CONFIG_RCU_BOOST) && !rcu_softirq_enabled)
> > return 0;
> > - WARN_ONCE(smpboot_register_percpu_thread(&rcu_cpu_thread_spec), "%s: Could not start rcub kthread, OOM is now expected behavior\n", __func__);
> > + WARN_ONCE(smpboot_register_percpu_thread(&rcu_cpu_thread_spec),
> > + "%s: Could not start rcuc kthread, OOM is now expected behavior\n", __func__);
> > return 0;
> > }
> > early_initcall(rcu_spawn_core_kthreads);
> > --
> > 2.20.1
> >
> > > - Joel
> >
> > Sebastian
>
Powered by blists - more mailing lists