linux-kernel - Re: [PATCH 05/16] rcu/nocb: Disable bypass when CPU isn't completely offloaded

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210128213133.GT2743@paulmck-ThinkPad-P72>
Date:   Thu, 28 Jan 2021 13:31:33 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Josh Triplett <josh@...htriplett.org>,
        Stable <stable@...r.kernel.org>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH 05/16] rcu/nocb: Disable bypass when CPU isn't completely
 offloaded

On Thu, Jan 28, 2021 at 06:12:11PM +0100, Frederic Weisbecker wrote:
> Instead of flushing bypass at the very last moment in the deoffloading
> process, just disable bypass enqueue at soon as we start the deoffloading
> process and flush the pending bypass early. It's less fragile and we
> leave some time to the kthreads and softirqs to process quietly.
> 
> Symmetrically, only enable bypass once we safely complete the offloading
> process.
> 
> Reported-by: Paul E. McKenney <paulmck@...nel.org>
> Cc: Josh Triplett <josh@...htriplett.org>
> Cc: Lai Jiangshan <jiangshanlai@...il.com>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: Neeraj Upadhyay <neeraju@...eaurora.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Frederic Weisbecker <frederic@...nel.org>

Looks plausible, thank you!  Some questions and comments below.

> ---
>  include/linux/rcu_segcblist.h |  7 ++++---
>  kernel/rcu/tree_plugin.h      | 31 +++++++++++++++++++++++--------
>  2 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> index 8afe886e85f1..5a2d6dadd720 100644
> --- a/include/linux/rcu_segcblist.h
> +++ b/include/linux/rcu_segcblist.h
> @@ -109,7 +109,7 @@ struct rcu_cblist {
>   *  |                           SEGCBLIST_KTHREAD_GP                           |
>   *  |                                                                          |
>   *  |   Kthreads handle callbacks holding nocb_lock, local rcu_core() stops    |
> - *  |   handling callbacks.                                                    |
> + *  |   handling callbacks. Allow bypass enqueue.                              |

"Allow bypass enqueue" as in bypass was disabled and entering this
state causes it to be enabled, correct?  If so, "Enable bypass
queueing" would be less ambiguous and would match the change below.

>   *  ----------------------------------------------------------------------------
>   */
>  
> @@ -125,7 +125,7 @@ struct rcu_cblist {
>   *  |                           SEGCBLIST_KTHREAD_GP                           |
>   *  |                                                                          |
>   *  |   CB/GP kthreads handle callbacks holding nocb_lock, local rcu_core()    |
> - *  |   ignores callbacks.                                                     |
> + *  |   ignores callbacks. Bypass enqueue is enabled.                          |
>   *  ----------------------------------------------------------------------------
>   *                                      |
>   *                                      v
> @@ -134,7 +134,8 @@ struct rcu_cblist {
>   *  |                           SEGCBLIST_KTHREAD_GP                           |
>   *  |                                                                          |
>   *  |   CB/GP kthreads and local rcu_core() handle callbacks concurrently      |
> - *  |   holding nocb_lock. Wake up CB and GP kthreads if necessary.            |
> + *  |   holding nocb_lock. Wake up CB and GP kthreads if necessary. Disable    |
> + *  |   bypass enqueue.                                                        |
>   *  ----------------------------------------------------------------------------
>   *                                      |
>   *                                      v
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 732942a15f2b..7781830a3cf1 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1825,11 +1825,22 @@ static bool rcu_nocb_try_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
>  	unsigned long j = jiffies;
>  	long ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
>  
> +	lockdep_assert_irqs_disabled();
> +
> +	// Pure softirq/rcuc based processing: no bypassing, no
> +	// locking.
>  	if (!rcu_rdp_is_offloaded(rdp)) {
> +		*was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
> +		return false;
> +	}
> +
> +	// In the process of (de-)offloading: no bypassing, but
> +	// locking.
> +	if (!rcu_segcblist_completely_offloaded(&rdp->cblist)) {
> +		rcu_nocb_lock(rdp);
>  		*was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
>  		return false; /* Not offloaded, no bypassing. */
>  	}
> -	lockdep_assert_irqs_disabled();
>  
>  	// Don't use ->nocb_bypass during early boot.
>  	if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING) {
> @@ -2412,6 +2423,7 @@ static long rcu_nocb_rdp_deoffload(void *arg)
>  
>  	rcu_nocb_lock_irqsave(rdp, flags);
>  
> +	WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));

This flush suffices because we are running on the target CPU
holding ->nocb_lock (thus having interrupts disabled), and
because rdp_offload_toggle() invokes rcu_segcblist_offload(),
which clears SEGCBLIST_OFFLOADED.  Thus future calls to
rcu_segcblist_completely_offloaded() will return false,
which means that future calls to rcu_nocb_try_bypass() will
refuse to put anything into the bypass.

I believe that this deserves a comment, particularly if I am
confused about what is really happening here.  ;-)

On another topic, since I saw it along the way...

The header comment for rcu_segcblist_offload() says that the
structure must be empty, but that isn't really the case, is it?

>  	ret = rdp_offload_toggle(rdp, false, flags);
>  	swait_event_exclusive(rdp->nocb_state_wq,
>  			      !rcu_segcblist_test_flags(cblist, SEGCBLIST_KTHREAD_CB |
> @@ -2422,19 +2434,22 @@ static long rcu_nocb_rdp_deoffload(void *arg)
>  	rcu_nocb_unlock_irqrestore(rdp, flags);
>  	del_timer_sync(&rdp->nocb_timer);
>  
> +	/* Sanity check */
> +	WARN_ON_ONCE(rcu_cblist_n_cbs(&rdp->nocb_bypass));
> +
>  	/*
> -	 * Flush bypass. While IRQs are disabled and once we set
> -	 * SEGCBLIST_SOFTIRQ_ONLY, no callback is supposed to be
> -	 * enqueued on bypass.
> +	 * Lock one last time so we see latest updates from kthreads and timer

You lost me here.  What updates are we seeing from kthreads and timers?

> +	 * so that we can later run callbacks locally without the lock.
>  	 */
>  	rcu_nocb_lock_irqsave(rdp, flags);
> -	rcu_nocb_flush_bypass(rdp, NULL, jiffies);
> +	/*
> +	 * Theoretically we could set SEGCBLIST_SOFTIRQ_ONLY after the nocb
> +	 * LOCK/UNLOCK but let's be paranoid.
> +	 */
>  	rcu_segcblist_set_flags(cblist, SEGCBLIST_SOFTIRQ_ONLY);

As long as we are being paranoid, should we also check that the
bypass remains empty?

>  	/*
>  	 * With SEGCBLIST_SOFTIRQ_ONLY, we can't use
> -	 * rcu_nocb_unlock_irqrestore() anymore. Theoretically we
> -	 * could set SEGCBLIST_SOFTIRQ_ONLY with cb unlocked and IRQs
> -	 * disabled now, but let's be paranoid.
> +	 * rcu_nocb_unlock_irqrestore() anymore.
>  	 */
>  	raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
>  
> -- 
> 2.25.1
>