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

[Valentin Schneider <valentin.schneider@arm.com>]

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@kernel.org>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> Cc: Uladzislau Rezki <urezki@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>

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

Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.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);