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: <20211004113551.GA271348@lothringen>
Date:   Mon, 4 Oct 2021 13:35:51 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     "Paul E . McKenney" <paulmck@...nel.org>,
        LKML <linux-kernel@...r.kernel.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 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@...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).

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.