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

[Frederic Weisbecker]

On Fri, Oct 01, 2021 at 06:48:28PM +0100, Valentin Schneider wrote:
> 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@xxxxxxxxxx>
> > Cc: Valentin Schneider <valentin.schneider@xxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> > Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Cc: Neeraj Upadhyay <neeraju@xxxxxxxxxxxxxx>
> > Cc: Uladzislau Rezki <urezki@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> One question and a comment nit below, other than that:
> 
> Reviewed-by: Valentin Schneider <Valentin.Schneider@xxxxxxx>
> 
> > @@ -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).

Ah thanks, I'll remove that.

> 
> >  {
> >       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.

Exactly :)

Thanks.