linux-kernel - Re: [PATCH RFC tip/core/rcu 1/2] rcu: Defer reporting RCU-preempt quiescent states when disabled

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: <20180702003553.GA95395@joelaf.mtv.corp.google.com>
Date:   Sun, 1 Jul 2018 17:35:53 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
        rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
        fweisbec@...il.com, oleg@...hat.com
Subject: Re: [PATCH RFC tip/core/rcu 1/2] rcu: Defer reporting RCU-preempt
 quiescent states when disabled

On Sun, Jul 01, 2018 at 03:27:49PM -0700, Paul E. McKenney wrote:
[...]
> > > +/*
> > > + * Report a deferred quiescent state if needed and safe to do so.
> > > + * As with rcu_preempt_need_deferred_qs(), "safe" involves only
> > > + * not being in an RCU read-side critical section.  The caller must
> > > + * evaluate safety in terms of interrupt, softirq, and preemption
> > > + * disabling.
> > > + */
> > > +static void rcu_preempt_deferred_qs(struct task_struct *t)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	if (!rcu_preempt_need_deferred_qs(t))
> > > +		return;
> > > +	local_irq_save(flags);
> > > +	rcu_preempt_deferred_qs_irqrestore(t, flags);
> > > +}
> > > +
> > > +/*
> > > + * Handle special cases during rcu_read_unlock(), such as needing to
> > > + * notify RCU core processing or task having blocked during the RCU
> > > + * read-side critical section.
> > > + */
> > > +static void rcu_read_unlock_special(struct task_struct *t)
> > > +{
> > > +	unsigned long flags;
> > > +	bool preempt_bh_were_disabled = !!(preempt_count() & ~HARDIRQ_MASK);
> > > +	bool irqs_were_disabled;
> > > +
> > > +	/* NMI handlers cannot block and cannot safely manipulate state. */
> > > +	if (in_nmi())
> > > +		return;
> > > +
> > > +	local_irq_save(flags);
> > > +	irqs_were_disabled = irqs_disabled_flags(flags);
> > > +	if ((preempt_bh_were_disabled || irqs_were_disabled) &&
> > > +	    t->rcu_read_unlock_special.b.blocked) {
> > > +		/* Need to defer quiescent state until everything is enabled. */
> > > +		raise_softirq_irqoff(RCU_SOFTIRQ);
> > > +		local_irq_restore(flags);
> > > +		return;
> > > +	}
> > > +	rcu_preempt_deferred_qs_irqrestore(t, flags);
> > > +}
> > > +
> > >  /*
> > >   * Dump detailed information for all tasks blocking the current RCU
> > >   * grace period on the specified rcu_node structure.
> > > @@ -737,10 +784,20 @@ static void rcu_preempt_check_callbacks(void)
> > >  	struct rcu_state *rsp = &rcu_preempt_state;
> > >  	struct task_struct *t = current;
> > >  
> > > -	if (t->rcu_read_lock_nesting == 0) {
> > > -		rcu_preempt_qs();
> > > +	if (t->rcu_read_lock_nesting > 0 ||
> > > +	    (preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK))) {
> > > +		/* No QS, force context switch if deferred. */
> > > +		if (rcu_preempt_need_deferred_qs(t))
> > > +			resched_cpu(smp_processor_id());
> > 
> > 
> > Hi Paul,
> > 
> > I had a similar idea of checking the preempt_count() sometime back but didn't
> > believe this path can be called with preempt enabled (for some reason ;-)).
> > Now that I've convinced myself that's possible, what do you think about
> > taking advantage of the opportunity to report a RCU-sched qs like below from
> > rcu_check_callbacks ?
> > 
> > Did some basic testing, can roll into a patch later if you're Ok with it.
> 
> The problem here is that the code patch above cannot be called
> with CONFIG_PREEMPT_COUNT=n, but the code below can.  And if
> CONFIG_PREEMPT_COUNT=n, the return value from preempt_count() can be
> misleading.
> 
> Or am I missing something here?

That is true! so then I could also test if PREEMPT_RCU is enabled like you're
doing in the other path.

thanks!

---8<-----------------------

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index fb440baf8ac6..03a460921dca 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2683,6 +2683,12 @@ void rcu_check_callbacks(int user)
 		rcu_note_voluntary_context_switch(current);
 
 	} else if (!in_softirq()) {
+		/*
+		 * Report RCU-sched qs if not in an RCU-sched read-side
+		 * critical section.
+		 */
+		if (IS_ENABLED(PREEMPT_RCU) && !(preempt_count() & PREEMPT_MASK))
+			rcu_sched_qs();
 
 		/*
 		 * Get here if this CPU did not take its interrupt from