Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1738070 > unrolled thread

[PATCH 1/4] rcu: Allow for page faults in NMI handlers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/4] rcu: Allow for page faults in NMI handlers Steven Rostedt <rostedt@goodmis.org> - 2017-09-23 23:00 +0200
    Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-24 21:50 +0200
      Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-25 02:10 +0200
        Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-25 02:20 +0200
          Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-25 02:30 +0200
            Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-25 02:40 +0200
              Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers Steven Rostedt <rostedt@goodmis.org> - 2017-09-25 06:50 +0200
                Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-25 07:00 +0200
                  Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-26 05:20 +0200

#1738070 — [PATCH 1/4] rcu: Allow for page faults in NMI handlers

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

A number of architecture invoke rcu_irq_enter() on exception entry in
order to allow RCU read-side critical sections in the exception handler
when the exception is from an idle or nohz_full CPU.  This works, at
least unless the exception happens in an NMI handler.  In that case,
rcu_nmi_enter() would already have exited the extended quiescent state,
which would mean that rcu_irq_enter() would (incorrectly) cause RCU
to think that it is again in an extended quiescent state.  This will
in turn result in lockdep splats in response to later RCU read-side
critical sections.

This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to
take no action if there is an rcu_nmi_enter() in effect, thus avoiding
the unscheduled return to RCU quiescent state.  This in turn should
make the kernel safe for on-demand RCU voyeurism.

Link: http://lkml.kernel.org/r/20170922211022.GA18084@linux.vnet.ibm.com

Cc: stable@vger.kernel.org
Fixes: 0be964be0 ("module: Sanitize RCU usage and locking")
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/rcu/tree.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 51d4c3acf32d..63bee8e1b193 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -888,6 +888,11 @@ void rcu_irq_exit(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
+		return;
+
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     rdtp->dynticks_nesting < 1);
 	if (rdtp->dynticks_nesting <= 1) {
@@ -1020,6 +1025,11 @@ void rcu_irq_enter(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
+		return;
+
 	oldval = rdtp->dynticks_nesting;
 	rdtp->dynticks_nesting++;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
-- 
2.13.2

[toc] | [next] | [standalone]

#1738256

On Sat, Sep 23, 2017 at 1:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> +
> +       /* Page faults can happen in NMI handlers, so check... */
> +       if (READ_ONCE(rdtp->dynticks_nmi_nesting))
> +               return;
> +

What is the reason for the READ_ONCE() here (and in the other case)?

It doesn't seem to have any actual reason.  It's a "stable" per-cpu
value in that even if an NMI were to happen, it gets incremented and
then decremented, so there is nothing really volatile about it
anywhere that I can see.

So the READ_ONCE() seems to be just pure confusion.

What am I missing?

               Linus

[toc] | [prev] | [next] | [standalone]

#1738604

On Sun, Sep 24, 2017 at 12:42:32PM -0700, Linus Torvalds wrote:
> On Sat, Sep 23, 2017 at 1:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > +
> > +       /* Page faults can happen in NMI handlers, so check... */
> > +       if (READ_ONCE(rdtp->dynticks_nmi_nesting))
> > +               return;
> > +
> 
> What is the reason for the READ_ONCE() here (and in the other case)?
> 
> It doesn't seem to have any actual reason.  It's a "stable" per-cpu
> value in that even if an NMI were to happen, it gets incremented and
> then decremented, so there is nothing really volatile about it
> anywhere that I can see.
> 
> So the READ_ONCE() seems to be just pure confusion.
> 
> What am I missing?

Mostly just paranoia on my part.  I would be happy to remove it if
you prefer.  Or you or Steve can do so if that is more convenient.

And yes, consistency would dictate that the uses in rcu_nmi_enter()
and rcu_nmi_exit() should be _ONCE(), particularly the stores to
->dynticks_nmi_nesting.  But I am not too worried about that right now
because I suspect that I should be able to combine rcu_irq_{enter,exit}()
and rcu_nmi_{enter,exit}(), which would be a good simplification.

							Thanx, aul

[toc] | [prev] | [next] | [standalone]

#1738608

On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> Mostly just paranoia on my part.  I would be happy to remove it if
> you prefer.  Or you or Steve can do so if that is more convenient.

I really don't think it's warranted. The values are *stable*. There's
no subtle lack of locking, or some optimistic access to a value that
can change.

The compiler can generate code to read the value fifteen billion
times, and it will always get the same value.

Yes, maybe in between the different accesses, an NMI will happen, and
the value will be incremented, but then as the NMI exits, it will
decrement again, so the code that got interrupted will not actually
see the change.

So the READ_ONCE() isn't "paranoia". It's just confusing.

> And yes, consistency would dictate that the uses in rcu_nmi_enter()
> and rcu_nmi_exit() should be _ONCE(), particularly the stores to
> ->dynticks_nmi_nesting.

NO.

That would be just more of that confusion.

That value is STABLE. It's stable even within an NMI handler. The NMI
code can read it, modify it, write it back, do a little dance, all
without having to care. There's no "_ONCE()" about it - not for the
readers, not for the writers, not for _anybody_.

So adding even more READ/WRITE_ONCE() accesses wouldn't be
"consistent", it would just be insanity.

Now, if an NMI happens and the value would be different on entry than
it is on exit, that would be something else. Then it really wouldn't
be stable wrt random users. But that would also be a major bug in the
NMI handler, as far as I can tell.

So the reason I'm objecting to that READ_ONCE() is that it isn't
"paranoia", it's "voodoo programming". And we don't do voodoo
programming.

            Linus

[toc] | [prev] | [next] | [standalone]

#1738609

On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote:
> On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > Mostly just paranoia on my part.  I would be happy to remove it if
> > you prefer.  Or you or Steve can do so if that is more convenient.
> 
> I really don't think it's warranted. The values are *stable*. There's
> no subtle lack of locking, or some optimistic access to a value that
> can change.
> 
> The compiler can generate code to read the value fifteen billion
> times, and it will always get the same value.
> 
> Yes, maybe in between the different accesses, an NMI will happen, and
> the value will be incremented, but then as the NMI exits, it will
> decrement again, so the code that got interrupted will not actually
> see the change.
> 
> So the READ_ONCE() isn't "paranoia". It's just confusing.
> 
> > And yes, consistency would dictate that the uses in rcu_nmi_enter()
> > and rcu_nmi_exit() should be _ONCE(), particularly the stores to
> > ->dynticks_nmi_nesting.
> 
> NO.
> 
> That would be just more of that confusion.
> 
> That value is STABLE. It's stable even within an NMI handler. The NMI
> code can read it, modify it, write it back, do a little dance, all
> without having to care. There's no "_ONCE()" about it - not for the
> readers, not for the writers, not for _anybody_.
> 
> So adding even more READ/WRITE_ONCE() accesses wouldn't be
> "consistent", it would just be insanity.
> 
> Now, if an NMI happens and the value would be different on entry than
> it is on exit, that would be something else. Then it really wouldn't
> be stable wrt random users. But that would also be a major bug in the
> NMI handler, as far as I can tell.
> 
> So the reason I'm objecting to that READ_ONCE() is that it isn't
> "paranoia", it's "voodoo programming". And we don't do voodoo
> programming.

I already agreed that the READ_ONCE() can be removed.

But without the WRITE_ONCE(), the compiler could theoretically tear
the store.  Now we might be asserting that our compilers don't do that,
and that if they ever do, we will file a bug or whatever.

So are we asserting that our compilers won't ever do store tearing?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1738610

On Sun, Sep 24, 2017 at 05:26:53PM -0700, Paul E. McKenney wrote:
> On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote:
> > On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > Mostly just paranoia on my part.  I would be happy to remove it if
> > > you prefer.  Or you or Steve can do so if that is more convenient.
> > 
> > I really don't think it's warranted. The values are *stable*. There's
> > no subtle lack of locking, or some optimistic access to a value that
> > can change.
> > 
> > The compiler can generate code to read the value fifteen billion
> > times, and it will always get the same value.
> > 
> > Yes, maybe in between the different accesses, an NMI will happen, and
> > the value will be incremented, but then as the NMI exits, it will
> > decrement again, so the code that got interrupted will not actually
> > see the change.
> > 
> > So the READ_ONCE() isn't "paranoia". It's just confusing.
> > 
> > > And yes, consistency would dictate that the uses in rcu_nmi_enter()
> > > and rcu_nmi_exit() should be _ONCE(), particularly the stores to
> > > ->dynticks_nmi_nesting.
> > 
> > NO.
> > 
> > That would be just more of that confusion.
> > 
> > That value is STABLE. It's stable even within an NMI handler. The NMI
> > code can read it, modify it, write it back, do a little dance, all
> > without having to care. There's no "_ONCE()" about it - not for the
> > readers, not for the writers, not for _anybody_.
> > 
> > So adding even more READ/WRITE_ONCE() accesses wouldn't be
> > "consistent", it would just be insanity.
> > 
> > Now, if an NMI happens and the value would be different on entry than
> > it is on exit, that would be something else. Then it really wouldn't
> > be stable wrt random users. But that would also be a major bug in the
> > NMI handler, as far as I can tell.
> > 
> > So the reason I'm objecting to that READ_ONCE() is that it isn't
> > "paranoia", it's "voodoo programming". And we don't do voodoo
> > programming.
> 
> I already agreed that the READ_ONCE() can be removed.

And for whatever it is worth, here is the updated patch.

							Thanx, Paul

------------------------------------------------------------------------

commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Fri Sep 22 13:14:42 2017 -0700

    rcu: Allow for page faults in NMI handlers
    
    A number of architecture invoke rcu_irq_enter() on exception entry in
    order to allow RCU read-side critical sections in the exception handler
    when the exception is from an idle or nohz_full CPU.  This works, at
    least unless the exception happens in an NMI handler.  In that case,
    rcu_nmi_enter() would already have exited the extended quiescent state,
    which would mean that rcu_irq_enter() would (incorrectly) cause RCU
    to think that it is again in an extended quiescent state.  This will
    in turn result in lockdep splats in response to later RCU read-side
    critical sections.
    
    This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to
    take no action if there is an rcu_nmi_enter() in effect, thus avoiding
    the unscheduled return to RCU quiescent state.  This in turn should
    make the kernel safe for on-demand RCU voyeurism.
    
    Reported-by: Steven Rostedt <rostedt@goodmis.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    [ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index db5eb8c3f7af..e4fe06d42385 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -891,6 +891,11 @@ void rcu_irq_exit(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (rdtp->dynticks_nmi_nesting)
+		return;
+
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     rdtp->dynticks_nesting < 1);
 	if (rdtp->dynticks_nesting <= 1) {
@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (rdtp->dynticks_nmi_nesting)
+		return;
+
 	oldval = rdtp->dynticks_nesting;
 	rdtp->dynticks_nesting++;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&

[toc] | [prev] | [next] | [standalone]

#1738735

Sorry for the top post, currently on a train to Paris.

This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?

Thanks,

-- Steve


On September 25, 2017 2:34:56 AM GMT+02:00, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>On Sun, Sep 24, 2017 at 05:26:53PM -0700, Paul E. McKenney wrote:
>> On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote:
>> > On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
>> > <paulmck@linux.vnet.ibm.com> wrote:
>> > >
>> > > Mostly just paranoia on my part.  I would be happy to remove it
>if
>> > > you prefer.  Or you or Steve can do so if that is more
>convenient.
>> > 
>> > I really don't think it's warranted. The values are *stable*.
>There's
>> > no subtle lack of locking, or some optimistic access to a value
>that
>> > can change.
>> > 
>> > The compiler can generate code to read the value fifteen billion
>> > times, and it will always get the same value.
>> > 
>> > Yes, maybe in between the different accesses, an NMI will happen,
>and
>> > the value will be incremented, but then as the NMI exits, it will
>> > decrement again, so the code that got interrupted will not actually
>> > see the change.
>> > 
>> > So the READ_ONCE() isn't "paranoia". It's just confusing.
>> > 
>> > > And yes, consistency would dictate that the uses in
>rcu_nmi_enter()
>> > > and rcu_nmi_exit() should be _ONCE(), particularly the stores to
>> > > ->dynticks_nmi_nesting.
>> > 
>> > NO.
>> > 
>> > That would be just more of that confusion.
>> > 
>> > That value is STABLE. It's stable even within an NMI handler. The
>NMI
>> > code can read it, modify it, write it back, do a little dance, all
>> > without having to care. There's no "_ONCE()" about it - not for the
>> > readers, not for the writers, not for _anybody_.
>> > 
>> > So adding even more READ/WRITE_ONCE() accesses wouldn't be
>> > "consistent", it would just be insanity.
>> > 
>> > Now, if an NMI happens and the value would be different on entry
>than
>> > it is on exit, that would be something else. Then it really
>wouldn't
>> > be stable wrt random users. But that would also be a major bug in
>the
>> > NMI handler, as far as I can tell.
>> > 
>> > So the reason I'm objecting to that READ_ONCE() is that it isn't
>> > "paranoia", it's "voodoo programming". And we don't do voodoo
>> > programming.
>> 
>> I already agreed that the READ_ONCE() can be removed.
>
>And for whatever it is worth, here is the updated patch.
>
>							Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43
>Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>Date:   Fri Sep 22 13:14:42 2017 -0700
>
>    rcu: Allow for page faults in NMI handlers
>    
>  A number of architecture invoke rcu_irq_enter() on exception entry in
>order to allow RCU read-side critical sections in the exception handler
>   when the exception is from an idle or nohz_full CPU.  This works, at
>   least unless the exception happens in an NMI handler.  In that case,
>rcu_nmi_enter() would already have exited the extended quiescent state,
>    which would mean that rcu_irq_enter() would (incorrectly) cause RCU
>   to think that it is again in an extended quiescent state.  This will
>    in turn result in lockdep splats in response to later RCU read-side
>    critical sections.
>    
>    This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to
> take no action if there is an rcu_nmi_enter() in effect, thus avoiding
>    the unscheduled return to RCU quiescent state.  This in turn should
>    make the kernel safe for on-demand RCU voyeurism.
>    
>    Reported-by: Steven Rostedt <rostedt@goodmis.org>
>    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>    [ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]
>
>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>index db5eb8c3f7af..e4fe06d42385 100644
>--- a/kernel/rcu/tree.c
>+++ b/kernel/rcu/tree.c
>@@ -891,6 +891,11 @@ void rcu_irq_exit(void)
> 
>	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs
>enabled!!!");
> 	rdtp = this_cpu_ptr(&rcu_dynticks);
>+
>+	/* Page faults can happen in NMI handlers, so check... */
>+	if (rdtp->dynticks_nmi_nesting)
>+		return;
>+
> 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> 		     rdtp->dynticks_nesting < 1);
> 	if (rdtp->dynticks_nesting <= 1) {
>@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void)
> 
>	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs
>enabled!!!");
> 	rdtp = this_cpu_ptr(&rcu_dynticks);
>+
>+	/* Page faults can happen in NMI handlers, so check... */
>+	if (rdtp->dynticks_nmi_nesting)
>+		return;
>+
> 	oldval = rdtp->dynticks_nesting;
> 	rdtp->dynticks_nesting++;
> 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1738736

On Mon, Sep 25, 2017 at 06:41:30AM +0200, Steven Rostedt wrote:
> Sorry for the top post, currently on a train to Paris.
> 
> This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?

If Linus accepts the original series, easy enough.

							Thanx, Paul

> Thanks,
> 
> -- Steve
> 
> 
> On September 25, 2017 2:34:56 AM GMT+02:00, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >On Sun, Sep 24, 2017 at 05:26:53PM -0700, Paul E. McKenney wrote:
> >> On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote:
> >> > On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
> >> > <paulmck@linux.vnet.ibm.com> wrote:
> >> > >
> >> > > Mostly just paranoia on my part.  I would be happy to remove it
> >if
> >> > > you prefer.  Or you or Steve can do so if that is more
> >convenient.
> >> > 
> >> > I really don't think it's warranted. The values are *stable*.
> >There's
> >> > no subtle lack of locking, or some optimistic access to a value
> >that
> >> > can change.
> >> > 
> >> > The compiler can generate code to read the value fifteen billion
> >> > times, and it will always get the same value.
> >> > 
> >> > Yes, maybe in between the different accesses, an NMI will happen,
> >and
> >> > the value will be incremented, but then as the NMI exits, it will
> >> > decrement again, so the code that got interrupted will not actually
> >> > see the change.
> >> > 
> >> > So the READ_ONCE() isn't "paranoia". It's just confusing.
> >> > 
> >> > > And yes, consistency would dictate that the uses in
> >rcu_nmi_enter()
> >> > > and rcu_nmi_exit() should be _ONCE(), particularly the stores to
> >> > > ->dynticks_nmi_nesting.
> >> > 
> >> > NO.
> >> > 
> >> > That would be just more of that confusion.
> >> > 
> >> > That value is STABLE. It's stable even within an NMI handler. The
> >NMI
> >> > code can read it, modify it, write it back, do a little dance, all
> >> > without having to care. There's no "_ONCE()" about it - not for the
> >> > readers, not for the writers, not for _anybody_.
> >> > 
> >> > So adding even more READ/WRITE_ONCE() accesses wouldn't be
> >> > "consistent", it would just be insanity.
> >> > 
> >> > Now, if an NMI happens and the value would be different on entry
> >than
> >> > it is on exit, that would be something else. Then it really
> >wouldn't
> >> > be stable wrt random users. But that would also be a major bug in
> >the
> >> > NMI handler, as far as I can tell.
> >> > 
> >> > So the reason I'm objecting to that READ_ONCE() is that it isn't
> >> > "paranoia", it's "voodoo programming". And we don't do voodoo
> >> > programming.
> >> 
> >> I already agreed that the READ_ONCE() can be removed.
> >
> >And for whatever it is worth, here is the updated patch.
> >
> >							Thanx, Paul
> >
> >------------------------------------------------------------------------
> >
> >commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43
> >Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >Date:   Fri Sep 22 13:14:42 2017 -0700
> >
> >    rcu: Allow for page faults in NMI handlers
> >    
> >  A number of architecture invoke rcu_irq_enter() on exception entry in
> >order to allow RCU read-side critical sections in the exception handler
> >   when the exception is from an idle or nohz_full CPU.  This works, at
> >   least unless the exception happens in an NMI handler.  In that case,
> >rcu_nmi_enter() would already have exited the extended quiescent state,
> >    which would mean that rcu_irq_enter() would (incorrectly) cause RCU
> >   to think that it is again in an extended quiescent state.  This will
> >    in turn result in lockdep splats in response to later RCU read-side
> >    critical sections.
> >    
> >    This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to
> > take no action if there is an rcu_nmi_enter() in effect, thus avoiding
> >    the unscheduled return to RCU quiescent state.  This in turn should
> >    make the kernel safe for on-demand RCU voyeurism.
> >    
> >    Reported-by: Steven Rostedt <rostedt@goodmis.org>
> >    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >    [ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]
> >
> >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >index db5eb8c3f7af..e4fe06d42385 100644
> >--- a/kernel/rcu/tree.c
> >+++ b/kernel/rcu/tree.c
> >@@ -891,6 +891,11 @@ void rcu_irq_exit(void)
> > 
> >	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs
> >enabled!!!");
> > 	rdtp = this_cpu_ptr(&rcu_dynticks);
> >+
> >+	/* Page faults can happen in NMI handlers, so check... */
> >+	if (rdtp->dynticks_nmi_nesting)
> >+		return;
> >+
> > 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > 		     rdtp->dynticks_nesting < 1);
> > 	if (rdtp->dynticks_nesting <= 1) {
> >@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void)
> > 
> >	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs
> >enabled!!!");
> > 	rdtp = this_cpu_ptr(&rcu_dynticks);
> >+
> >+	/* Page faults can happen in NMI handlers, so check... */
> >+	if (rdtp->dynticks_nmi_nesting)
> >+		return;
> >+
> > 	oldval = rdtp->dynticks_nesting;
> > 	rdtp->dynticks_nesting++;
> > 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> 

[toc] | [prev] | [next] | [standalone]

#1739480

On Sun, Sep 24, 2017 at 09:56:32PM -0700, Paul E. McKenney wrote:
> On Mon, Sep 25, 2017 at 06:41:30AM +0200, Steven Rostedt wrote:
> > Sorry for the top post, currently on a train to Paris.
> > 
> > This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?
> 
> If Linus accepts the original series, easy enough.

And he did, so here is the READ_ONCE()-removal commit that I have queued
for the next merge window.  If anyone feels it is needed sooner, please
let me know.  (Can't see why it is urgent myself, but who knows...)

							Thanx, Paul

------------------------------------------------------------------------

commit 79e6337d3f3a629be48cd45d5075d058788ce90f
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Sep 25 20:07:49 2017 -0700

    rcu: Remove extraneous READ_ONCE()s from rcu_irq_{enter,exit}()
    
    The read of ->dynticks_nmi_nesting in rcu_irq_enter() and rcu_irq_exit()
    is currently protected with READ_ONCE().  However, this protection is
    unnecessary because (1) ->dynticks_nmi_nesting is updated only by the
    current CPU, (2) Although NMI handlers can update this field, they reset
    it back to its old value before return, and (3) Interrupts are disabled,
    so nothing else can modify it.  The value of ->dynticks_nmi_nesting is
    thus effectively constant, and so no protection is required.
    
    This commit therefore removes the READ_ONCE() protection from these
    two accesses.
    
    Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 872d20cee00a..e4fe06d42385 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -893,7 +893,7 @@ void rcu_irq_exit(void)
 	rdtp = this_cpu_ptr(&rcu_dynticks);
 
 	/* Page faults can happen in NMI handlers, so check... */
-	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
+	if (rdtp->dynticks_nmi_nesting)
 		return;
 
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
@@ -1043,7 +1043,7 @@ void rcu_irq_enter(void)
 	rdtp = this_cpu_ptr(&rcu_dynticks);
 
 	/* Page faults can happen in NMI handlers, so check... */
-	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
+	if (rdtp->dynticks_nmi_nesting)
 		return;
 
 	oldval = rdtp->dynticks_nesting;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web