LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Paul E. McKenney" <paulmck@linux.ibm.com>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
	jiangshanlai@gmail.com, dipankar@in.ibm.com,
	mathieu.desnoyers@efficios.com, josh@joshtriplett.org,
	rostedt@goodmis.org, luto@kernel.org, byungchul.park@lge.com
Subject: Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts
Date: Mon, 11 Mar 2019 15:29:03 -0700
Message-ID: <20190311222903.GR13351@linux.ibm.com> (raw)
In-Reply-To: <20190311133939.GA29747@google.com>

On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > on the other.  These things really did happen at one time, as evidenced
> > by this ca-2011 LKML post:
> > 
> > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > 
> > The reason why RCU tolerates half-interrupts is that usermode helpers
> > used exceptions to invoke a system call from within the kernel such that
> > the system call did a normal return (not a return from exception) to
> > the calling context.  This caused rcu_irq_enter() to be invoked without
> > a matching rcu_irq_exit().  However, usermode helpers have since been
> > rewritten to make much more housebroken use of workqueues, kernel threads,
> > and do_execve(), and therefore should no longer produce half-interrupts.
> > No one knows of any other source of half-interrupts, but then again,
> > no one seems insane enough to go audit the entire kernel to verify that
> > half-interrupts really are a relic of the past.
> > 
> > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > trigger in the presence of half interrupts, which the code will continue
> > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > would be a considerable simplification.
> 
> Hi Paul and everyone,
> I was thinking some more about this patch and whether we can simplify this code
> much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> keep it fresh in memory is a good idea ;-)

Indeed, easy to forget.  ;-)

> To me it seems we cannot easily combine the counters (dynticks_nesting and
> dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> half-interrupt scenario (assuming simplication means counter combining like
> Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> 2 counters need to be tracked separately as they are used differently in the
> following function:
> 
> static int rcu_is_cpu_rrupt_from_idle(void)
> {
>         return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
>                __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> }
> 
> dynticks_nesting actually tracks if we entered/exited idle or user mode.

True, though it tracks user mode only in CONFIG_NO_HZ_FULL kernels.

> dynticks_nmi_nesting tracks if we entered/exited interrupts.

Including NMIs, yes.

> We have to do the "dynticks_nmi_nesting <= 1" check because
> rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
> (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
> check is because the CPU MUST be in user or idle for the check to return
> true. We can't really combine these two into one counter then I think because
> they both convey different messages.
> 
> The only simplication we can do, is probably the "crowbar" updates to
> dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
> no more half-interrupts are possible. Which might still be a worthwhile thing
> to do (while still keeping both counters separate).
> 
> However, I think we could combine the counters and lead to simplying the code
> in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
> not need the counters but NOHZ_FULL may take issue with that since it needs
> rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.

I haven't gone through it in detail, but it seems like we should be able
to treat in-kernel process-level execution like an interrupt from idle
or userspace, as the case might be.  If we did that, shouldn't we be
able to just do this?

static int rcu_is_cpu_rrupt_from_idle(void)
{
        return __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
}

> Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
> In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
> will really get fired for half-interrupts. The vast majority of the systems I believe are
> NOHZ_IDLE not NOHZ_FULL.
> This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
> rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
> rcu_user_enter()  [due to usermode upcall]
> rcu_user_exit()
> (no more rcu_irq_exit() - hence half an interrupt)
> 
> But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
> rcu_eqs_e{xit,nter} really do anything?

Yes, because these are also called from rcu_idle_enter() and
rcu_idle_exit(), which is invoked even in !NO_HZ_FULL kernels.

> Or was the idea with adding the new warnings, that they would fire the next
> time rcu_idle_enter/exit is called? Like for example:
> 
> rcu_irq_enter()   [This is due to half-interrupt]
> rcu_idle_enter()  [Eventually we enter the idle loop at some point
> 		   after the half-interrupt and the rcu_eqs_enter()
> 		   would "crowbar" the dynticks_nmi_nesting counter to 0].

You got it!  ;-)

So yes, these warnings just detect the presence of misnesting.  Presumably
event tracing would then be used to track down the culprits.  Assuming that
the misnesting is reproducible and all that.

							Thanx, Paul

> thanks!
> 
>  - Joel
> 
> > 
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Reported-by: Joel Fernandes <joel@joelfernandes.org>
> > Reported-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index dc041c2afbcc..d2b6ade692c9 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
> >  	struct rcu_dynticks *rdtp;
> >  
> >  	rdtp = this_cpu_ptr(&rcu_dynticks);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
> >  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> >  		     rdtp->dynticks_nesting == 0);
> > @@ -895,6 +896,7 @@ static void rcu_eqs_exit(bool user)
> >  	trace_rcu_dyntick(TPS("End"), rdtp->dynticks_nesting, 1, rdtp->dynticks);
> >  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> >  	WRITE_ONCE(rdtp->dynticks_nesting, 1);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> >  }
> >  
> > -- 
> > 2.17.1
> > 
> 


  reply index

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-29 22:20 [PATCH tip/core/rcu 0/19] RCU flavor-consolidation changes for v4.20/v5.0 Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 01/19] rcu: Refactor rcu_{nmi,irq}_{enter,exit}() Paul E. McKenney
2018-08-30 18:10   ` Steven Rostedt
2018-08-30 23:02     ` Paul E. McKenney
2018-08-31  2:25     ` Byungchul Park
2018-08-29 22:20 ` [PATCH tip/core/rcu 02/19] rcu: Defer reporting RCU-preempt quiescent states when disabled Paul E. McKenney
2018-10-29 11:24   ` Ran Rozenstein
2018-10-29 14:27     ` Paul E. McKenney
2018-10-30  3:44       ` Joel Fernandes
2018-10-30 12:58         ` Paul E. McKenney
2018-10-30 22:21           ` Joel Fernandes
2018-10-31 18:22             ` Paul E. McKenney
2018-11-02 19:43               ` Paul E. McKenney
2018-11-26 13:55                 ` Ran Rozenstein
2018-11-26 19:00                   ` Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 03/19] rcutorture: Test extended "rcu" read-side critical sections Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 04/19] rcu: Allow processing deferred QSes for exiting RCU-preempt readers Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 05/19] rcu: Remove now-unused ->b.exp_need_qs field from the rcu_special union Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts Paul E. McKenney
2019-03-11 13:39   ` Joel Fernandes
2019-03-11 22:29     ` Paul E. McKenney [this message]
2019-03-12 15:05       ` Joel Fernandes
2019-03-12 15:20         ` Paul E. McKenney
2019-03-13 15:09           ` Joel Fernandes
2019-03-13 15:27             ` Steven Rostedt
2019-03-13 15:51               ` Paul E. McKenney
2019-03-13 16:51                 ` Steven Rostedt
2019-03-13 18:07                   ` Paul E. McKenney
2019-03-14 12:31                     ` Joel Fernandes
2019-03-14 13:36                       ` Steven Rostedt
2019-03-14 13:37                         ` Steven Rostedt
2019-03-14 21:27                           ` Joel Fernandes
2019-03-15  7:31     ` Byungchul Park
2019-03-15  7:44       ` Byungchul Park
2019-03-15 13:46         ` Joel Fernandes
2018-08-29 22:20 ` [PATCH tip/core/rcu 07/19] rcu: Apply RCU-bh QSes to RCU-sched and RCU-preempt when safe Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 08/19] rcu: Report expedited grace periods at context-switch time Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 09/19] rcu: Define RCU-bh update API in terms of RCU Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 10/19] rcu: Update comments and help text for no more RCU-bh updaters Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 11/19] rcu: Drop "wake" parameter from rcu_report_exp_rdp() Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 12/19] rcu: Fix typo in rcu_get_gp_kthreads_prio() header comment Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 13/19] rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 14/19] rcu: Express Tiny RCU updates in terms of RCU rather than RCU-sched Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 15/19] rcu: Remove RCU_STATE_INITIALIZER() Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 16/19] rcu: Eliminate rcu_state structure's ->call field Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 17/19] rcu: Remove rcu_state structure's ->rda field Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 18/19] rcu: Remove rcu_state_p pointer to default rcu_state structure Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 19/19] rcu: Remove rcu_data_p pointer to default rcu_data structure Paul E. McKenney
2018-08-29 22:22 ` [PATCH tip/core/rcu 0/19] RCU flavor-consolidation changes for v4.20/v5.0 Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190311222903.GR13351@linux.ibm.com \
    --to=paulmck@linux.ibm.com \
    --cc=byungchul.park@lge.com \
    --cc=dipankar@in.ibm.com \
    --cc=jiangshanlai@gmail.com \
    --cc=joel@joelfernandes.org \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=rcu@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git