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.
