linux-kernel - Re: [PATCH 02/11] rcu/nocb: Prepare state machine for a new step

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ee94myab.mognet@arm.com>
Date:   Fri, 01 Oct 2021 18:48:28 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Frederic Weisbecker <frederic@...nel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Uladzislau Rezki <urezki@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Boqun Feng <boqun.feng@...il.com>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Josh Triplett <josh@...htriplett.org>,
        Joel Fernandes <joel@...lfernandes.org>, rcu@...r.kernel.org
Subject: Re: [PATCH 02/11] rcu/nocb: Prepare state machine for a new step

On 30/09/21 00:10, Frederic Weisbecker wrote:
> Currently SEGCBLIST_SOFTIRQ_ONLY is a bit of an exception among the
> segcblist flags because it is an exclusive state that doesn't mix up
> with the other flags. Remove it in favour of:
>
> _ A flag specifying that rcu_core() needs to perform callbacks execution
>   and acceleration
>
> and
>
> _ A flag specifying we want the nocb lock to be held in any needed
>   circumstances
>
> This clarifies the code and is more flexible: It allows to have a state
> where rcu_core() runs with locking while offloading hasn't started yet.
> This is a necessary step to prepare for triggering rcu_core() at the
> very beginning of the de-offloading process so that rcu_core() won't
> dismiss work while being preempted by the de-offloading process, at
> least not without a pending subsequent rcu_core() that will quickly
> catch up.
>
> Signed-off-by: Frederic Weisbecker <frederic@...nel.org>
> Cc: Valentin Schneider <valentin.schneider@....com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: Josh Triplett <josh@...htriplett.org>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Neeraj Upadhyay <neeraju@...eaurora.org>
> Cc: Uladzislau Rezki <urezki@...il.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>

One question and a comment nit below, other than that:

Reviewed-by: Valentin Schneider <Valentin.Schneider@....com>

> @@ -84,7 +84,7 @@ static inline bool rcu_segcblist_is_enabled(struct rcu_segcblist *rsclp)
>  static inline bool rcu_segcblist_is_offloaded(struct rcu_segcblist *rsclp)

It doesn't show up on the diff but there's a SEGCBLIST_SOFTIRQ_ONLY
straggler in the comment above (the last one according to grep).

>  {
>       if (IS_ENABLED(CONFIG_RCU_NOCB_CPU) &&
> -	    !rcu_segcblist_test_flags(rsclp, SEGCBLIST_SOFTIRQ_ONLY))
> +	    rcu_segcblist_test_flags(rsclp, SEGCBLIST_LOCKING))
>               return true;
>
>       return false;

> @@ -1000,12 +1000,12 @@ static long rcu_nocb_rdp_deoffload(void *arg)
>        */
>       rcu_nocb_lock_irqsave(rdp, flags);
>       /*
> -	 * Theoretically we could set SEGCBLIST_SOFTIRQ_ONLY after the nocb
> +	 * Theoretically we could clear SEGCBLIST_LOCKING after the nocb
>        * lock is released but how about being paranoid for once?
>        */
> -	rcu_segcblist_set_flags(cblist, SEGCBLIST_SOFTIRQ_ONLY);
> +	rcu_segcblist_clear_flags(cblist, SEGCBLIST_LOCKING);

Thought experiment for me; AFAICT the comment still holds: we can't offload
until deoffload has finished, and we shouldn't be able to preempt
rcu_core() while it holds ->nocb_lock. With that said, I'm all for
paranoia.

>       /*
> -	 * With SEGCBLIST_SOFTIRQ_ONLY, we can't use
> +	 * Without SEGCBLIST_LOCKING, we can't use
>        * rcu_nocb_unlock_irqrestore() anymore.
>        */
>       raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);