A KCSAN build revealed we have explicit annoations through atomic_*()
usage, switch to arch_atomic_*() for the respective functions.
vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/rcu/tree.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
* next idle sojourn.
*/
rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is no longer watching. Better be in extended quiescent state!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_CTR));
@@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
* and we also must force ordering with the next RCU read-side
* critical section.
*/
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is now watching. Better not be in an extended quiescent state!
rcu_dynticks_task_trace_exit(); // After ->dynticks update!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
!(seq & RCU_DYNTICK_CTRL_CTR));
if (seq & RCU_DYNTICK_CTRL_MASK) {
- atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
+ arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
smp_mb__after_atomic(); /* _exit after clearing mask. */
}
}
@@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
- return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
+ return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
}
/*
@@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
+ instrumentation_begin();
/*
* Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
* (We are exiting an NMI handler, so RCU better be paying attention
@@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
* leave it in non-RCU-idle state.
*/
if (rdp->dynticks_nmi_nesting != 1) {
- instrumentation_begin();
trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
@@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
return;
}
- instrumentation_begin();
/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
On Wed, Jun 03, 2020 at 01:40:16PM +0200, Peter Zijlstra wrote:
> A KCSAN build revealed we have explicit annoations through atomic_*()
> usage, switch to arch_atomic_*() for the respective functions.
>
> vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> kernel/rcu/tree.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> * next idle sojourn.
> */
> rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
To preserve KCSAN's ability to see this, there would be something like
instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) prior
to the instrumentation_end() invoked before rcu_dynticks_eqs_enter()
in each of rcu_eqs_enter() and rcu_nmi_exit(), correct?
> // RCU is no longer watching. Better be in extended quiescent state!
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> (seq & RCU_DYNTICK_CTRL_CTR));
> @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> * and we also must force ordering with the next RCU read-side
> * critical section.
> */
> - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
And same here, but after the instrumentation_begin() following
rcu_dynticks_eqs_exit() in both rcu_eqs_exit() and rcu_nmi_enter(),
correct?
> // RCU is now watching. Better not be in an extended quiescent state!
> rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> !(seq & RCU_DYNTICK_CTRL_CTR));
> if (seq & RCU_DYNTICK_CTRL_MASK) {
> - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
This one is gone in -rcu.
> smp_mb__after_atomic(); /* _exit after clearing mask. */
> }
> }
> @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
follows:
o rcu_nmi_exit(): After each following instrumentation_begin().
o In theory in rcu_irq_exit_preempt(), but as this generates code
only in lockdep builds, it might not be worth worrying about.
o Ditto for rcu_irq_exit_check_preempt().
o Ditto for __rcu_irq_enter_check_tick().
o rcu_nmi_enter(): After each following instrumentation_begin().
o __rcu_is_watching() is itself noinstr:
o idtentry_enter_cond_rcu(): After each following
instrumentation_begin().
o rcu_is_watching(): Either before or after the call to
rcu_dynticks_curr_cpu_in_eqs().
> }
>
> /*
> @@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> + instrumentation_begin();
> /*
> * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> * (We are exiting an NMI handler, so RCU better be paying attention
> @@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
> * leave it in non-RCU-idle state.
> */
> if (rdp->dynticks_nmi_nesting != 1) {
> - instrumentation_begin();
> trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> atomic_read(&rdp->dynticks));
> WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> @@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
> return;
> }
>
> - instrumentation_begin();
> /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
This one looks to be having no effect on instrumentation of atomics, but
rather coalescing a pair of instrumentation_begin() into one.
Do I understand correctly?
Thanx, Paul
On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > * next idle sojourn.
> > */
> > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
>
> To preserve KCSAN's ability to see this, there would be something like
> instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) prior
> to the instrumentation_end() invoked before rcu_dynticks_eqs_enter()
> in each of rcu_eqs_enter() and rcu_nmi_exit(), correct?
Yes.
> > // RCU is no longer watching. Better be in extended quiescent state!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > (seq & RCU_DYNTICK_CTRL_CTR));
> > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > * and we also must force ordering with the next RCU read-side
> > * critical section.
> > */
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
>
> And same here, but after the instrumentation_begin() following
> rcu_dynticks_eqs_exit() in both rcu_eqs_exit() and rcu_nmi_enter(),
> correct?
Yep.
> > // RCU is now watching. Better not be in an extended quiescent state!
> > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > !(seq & RCU_DYNTICK_CTRL_CTR));
> > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
>
> This one is gone in -rcu.
Good, because that would make things 'complicated' with the external
instrumentation call. And is actually the reason I didn't even attempt
it this time around.
> > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > }
> > }
> > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
The above is actually instrumented by KCSAN, due to arch_atomic_read()
being a READ_ONCE() and it now understanding volatile.
> Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
> follows:
>
> o rcu_nmi_exit(): After each following instrumentation_begin().
Yes
> o In theory in rcu_irq_exit_preempt(), but as this generates code
> only in lockdep builds, it might not be worth worrying about.
>
> o Ditto for rcu_irq_exit_check_preempt().
>
> o Ditto for __rcu_irq_enter_check_tick().
Not these, afaict they're all the above arch_atomic_read(), which is
instrumented due to volatile in these cases.
> o rcu_nmi_enter(): After each following instrumentation_begin().
Yes
> o __rcu_is_watching() is itself noinstr:
>
> o idtentry_enter_cond_rcu(): After each following
> instrumentation_begin().
>
> o rcu_is_watching(): Either before or after the call to
> rcu_dynticks_curr_cpu_in_eqs().
Something like that yes.
> > }
> >
> > /*
> > @@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > + instrumentation_begin();
> > /*
> > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > * (We are exiting an NMI handler, so RCU better be paying attention
> > @@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
> > * leave it in non-RCU-idle state.
> > */
> > if (rdp->dynticks_nmi_nesting != 1) {
> > - instrumentation_begin();
> > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > @@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
> > return;
> > }
> >
> > - instrumentation_begin();
> > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
>
> This one looks to be having no effect on instrumentation of atomics, but
> rather coalescing a pair of instrumentation_begin() into one.
>
> Do I understand correctly?
Almost, it puts the WARN_ON_ONCE()s under instrumentation_begin() too,
and that makes a differnce, iirc it was the
rcu_dynticks_curr_cpu_in_eqs() call that stood out. But that could've
been before I switched it to arch_atomic_read(). In any case, I find
this form a lot clearer.
On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
>
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > > * next idle sojourn.
> > > */
> > > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> >
> > To preserve KCSAN's ability to see this, there would be something like
> > instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) prior
> > to the instrumentation_end() invoked before rcu_dynticks_eqs_enter()
> > in each of rcu_eqs_enter() and rcu_nmi_exit(), correct?
>
> Yes.
>
> > > // RCU is no longer watching. Better be in extended quiescent state!
> > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > (seq & RCU_DYNTICK_CTRL_CTR));
> > > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > > * and we also must force ordering with the next RCU read-side
> > > * critical section.
> > > */
> > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> >
> > And same here, but after the instrumentation_begin() following
> > rcu_dynticks_eqs_exit() in both rcu_eqs_exit() and rcu_nmi_enter(),
> > correct?
>
> Yep.
>
> > > // RCU is now watching. Better not be in an extended quiescent state!
> > > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > !(seq & RCU_DYNTICK_CTRL_CTR));
> > > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> >
> > This one is gone in -rcu.
>
> Good, because that would make things 'complicated' with the external
> instrumentation call. And is actually the reason I didn't even attempt
> it this time around.
>
> > > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > > }
> > > }
> > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > {
> > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >
> > > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
>
> The above is actually instrumented by KCSAN, due to arch_atomic_read()
> being a READ_ONCE() and it now understanding volatile.
>
> > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
Right, this should instead be instrument_read(...).
Though if KCSAN is unconditionally instrumenting volatile, how does
this help? Or does KCSAN's instrumentation of volatile somehow avoid
causing trouble?
> > follows:
> >
> > o rcu_nmi_exit(): After each following instrumentation_begin().
>
> Yes
>
> > o In theory in rcu_irq_exit_preempt(), but as this generates code
> > only in lockdep builds, it might not be worth worrying about.
> >
> > o Ditto for rcu_irq_exit_check_preempt().
> >
> > o Ditto for __rcu_irq_enter_check_tick().
>
> Not these, afaict they're all the above arch_atomic_read(), which is
> instrumented due to volatile in these cases.
>
> > o rcu_nmi_enter(): After each following instrumentation_begin().
>
> Yes
>
> > o __rcu_is_watching() is itself noinstr:
> >
> > o idtentry_enter_cond_rcu(): After each following
> > instrumentation_begin().
> >
> > o rcu_is_watching(): Either before or after the call to
> > rcu_dynticks_curr_cpu_in_eqs().
>
> Something like that yes.
>
> > > }
> > >
> > > /*
> > > @@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
> > > {
> > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >
> > > + instrumentation_begin();
> > > /*
> > > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > > * (We are exiting an NMI handler, so RCU better be paying attention
> > > @@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
> > > * leave it in non-RCU-idle state.
> > > */
> > > if (rdp->dynticks_nmi_nesting != 1) {
> > > - instrumentation_begin();
> > > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > > atomic_read(&rdp->dynticks));
> > > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > > @@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
> > > return;
> > > }
> > >
> > > - instrumentation_begin();
> > > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> >
> > This one looks to be having no effect on instrumentation of atomics, but
> > rather coalescing a pair of instrumentation_begin() into one.
> >
> > Do I understand correctly?
>
> Almost, it puts the WARN_ON_ONCE()s under instrumentation_begin() too,
> and that makes a differnce, iirc it was the
> rcu_dynticks_curr_cpu_in_eqs() call that stood out. But that could've
> been before I switched it to arch_atomic_read(). In any case, I find
> this form a lot clearer.
Got it, thank you.
Thanx, Paul
On Thu, 4 Jun 2020 at 05:34, Paul E. McKenney <[email protected]> wrote:
>
> On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> > On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
> >
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > > > * next idle sojourn.
> > > > */
> > > > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > >
> > > To preserve KCSAN's ability to see this, there would be something like
> > > instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) prior
> > > to the instrumentation_end() invoked before rcu_dynticks_eqs_enter()
> > > in each of rcu_eqs_enter() and rcu_nmi_exit(), correct?
> >
> > Yes.
> >
> > > > // RCU is no longer watching. Better be in extended quiescent state!
> > > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > > (seq & RCU_DYNTICK_CTRL_CTR));
> > > > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > > > * and we also must force ordering with the next RCU read-side
> > > > * critical section.
> > > > */
> > > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > >
> > > And same here, but after the instrumentation_begin() following
> > > rcu_dynticks_eqs_exit() in both rcu_eqs_exit() and rcu_nmi_enter(),
> > > correct?
> >
> > Yep.
> >
> > > > // RCU is now watching. Better not be in an extended quiescent state!
> > > > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > > !(seq & RCU_DYNTICK_CTRL_CTR));
> > > > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > > > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > >
> > > This one is gone in -rcu.
> >
> > Good, because that would make things 'complicated' with the external
> > instrumentation call. And is actually the reason I didn't even attempt
> > it this time around.
> >
> > > > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > > > }
> > > > }
> > > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > > {
> > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >
> > > > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> >
> > The above is actually instrumented by KCSAN, due to arch_atomic_read()
> > being a READ_ONCE() and it now understanding volatile.
> >
> > > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
>
> Right, this should instead be instrument_read(...).
>
> Though if KCSAN is unconditionally instrumenting volatile, how does
> this help? Or does KCSAN's instrumentation of volatile somehow avoid
> causing trouble?
When used normally outside noinstr functions, because this is an
__always_inline function, it will be instrumented. Within noinstr
(which imply __no_kcsan) functions it should not be instrumented.
Thanks,
-- Marco
> > > follows:
> > >
> > > o rcu_nmi_exit(): After each following instrumentation_begin().
> >
> > Yes
> >
> > > o In theory in rcu_irq_exit_preempt(), but as this generates code
> > > only in lockdep builds, it might not be worth worrying about.
> > >
> > > o Ditto for rcu_irq_exit_check_preempt().
> > >
> > > o Ditto for __rcu_irq_enter_check_tick().
> >
> > Not these, afaict they're all the above arch_atomic_read(), which is
> > instrumented due to volatile in these cases.
> >
> > > o rcu_nmi_enter(): After each following instrumentation_begin().
> >
> > Yes
> >
> > > o __rcu_is_watching() is itself noinstr:
> > >
> > > o idtentry_enter_cond_rcu(): After each following
> > > instrumentation_begin().
> > >
> > > o rcu_is_watching(): Either before or after the call to
> > > rcu_dynticks_curr_cpu_in_eqs().
> >
> > Something like that yes.
> >
> > > > }
> > > >
> > > > /*
> > > > @@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
> > > > {
> > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >
> > > > + instrumentation_begin();
> > > > /*
> > > > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > > > * (We are exiting an NMI handler, so RCU better be paying attention
> > > > @@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
> > > > * leave it in non-RCU-idle state.
> > > > */
> > > > if (rdp->dynticks_nmi_nesting != 1) {
> > > > - instrumentation_begin();
> > > > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > > > atomic_read(&rdp->dynticks));
> > > > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > > > @@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
> > > > return;
> > > > }
> > > >
> > > > - instrumentation_begin();
> > > > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > > > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > > > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > >
> > > This one looks to be having no effect on instrumentation of atomics, but
> > > rather coalescing a pair of instrumentation_begin() into one.
> > >
> > > Do I understand correctly?
> >
> > Almost, it puts the WARN_ON_ONCE()s under instrumentation_begin() too,
> > and that makes a differnce, iirc it was the
> > rcu_dynticks_curr_cpu_in_eqs() call that stood out. But that could've
> > been before I switched it to arch_atomic_read(). In any case, I find
> > this form a lot clearer.
>
> Got it, thank you.
>
> Thanx, Paul
On Wed, Jun 03, 2020 at 08:34:09PM -0700, Paul E. McKenney wrote:
> On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> > On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
> > > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > > {
> > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >
> > > > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> >
> > The above is actually instrumented by KCSAN, due to arch_atomic_read()
> > being a READ_ONCE() and it now understanding volatile.
> >
> > > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
>
> Right, this should instead be instrument_read(...).
>
> Though if KCSAN is unconditionally instrumenting volatile, how does
> this help? Or does KCSAN's instrumentation of volatile somehow avoid
> causing trouble?
As Marco already explained, when used inside noinstr no instrumentation
will be emitted, when used outside noinstr it will emit the right
instrumentation.
> > > o In theory in rcu_irq_exit_preempt(), but as this generates code
> > > only in lockdep builds, it might not be worth worrying about.
> > >
> > > o Ditto for rcu_irq_exit_check_preempt().
> > >
> > > o Ditto for __rcu_irq_enter_check_tick().
> >
> > Not these, afaict they're all the above arch_atomic_read(), which is
> > instrumented due to volatile in these cases.
I this case, the above call-sites are all not noinstr (double negative!)
and will thus cause instrumentation to be emitted.
This is all a 'special' case for arch_atomic_read() (and _set()),
because they're basically READ_ONCE() (and WRITE_ONCE() resp.). The
normal atomics are asm() and it doesn't do anything for those (although
I suppose clang could, since it has this internal assembler to parse the
inline asm, but afaiu that's not something GCC ever wants to do).
On Thu, Jun 04, 2020 at 08:02:31AM +0200, Marco Elver wrote:
> On Thu, 4 Jun 2020 at 05:34, Paul E. McKenney <[email protected]> wrote:
> >
> > On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> > > On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
> > >
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > > > > * next idle sojourn.
> > > > > */
> > > > > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > > > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > >
> > > > To preserve KCSAN's ability to see this, there would be something like
> > > > instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) prior
> > > > to the instrumentation_end() invoked before rcu_dynticks_eqs_enter()
> > > > in each of rcu_eqs_enter() and rcu_nmi_exit(), correct?
> > >
> > > Yes.
> > >
> > > > > // RCU is no longer watching. Better be in extended quiescent state!
> > > > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > > > (seq & RCU_DYNTICK_CTRL_CTR));
> > > > > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > > > > * and we also must force ordering with the next RCU read-side
> > > > > * critical section.
> > > > > */
> > > > > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > > > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > >
> > > > And same here, but after the instrumentation_begin() following
> > > > rcu_dynticks_eqs_exit() in both rcu_eqs_exit() and rcu_nmi_enter(),
> > > > correct?
> > >
> > > Yep.
> > >
> > > > > // RCU is now watching. Better not be in an extended quiescent state!
> > > > > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > > > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > > > !(seq & RCU_DYNTICK_CTRL_CTR));
> > > > > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > > > > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > > > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > >
> > > > This one is gone in -rcu.
> > >
> > > Good, because that would make things 'complicated' with the external
> > > instrumentation call. And is actually the reason I didn't even attempt
> > > it this time around.
> > >
> > > > > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > > > > }
> > > > > }
> > > > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > > > {
> > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > >
> > > > > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > > > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > >
> > > The above is actually instrumented by KCSAN, due to arch_atomic_read()
> > > being a READ_ONCE() and it now understanding volatile.
> > >
> > > > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
> >
> > Right, this should instead be instrument_read(...).
> >
> > Though if KCSAN is unconditionally instrumenting volatile, how does
> > this help? Or does KCSAN's instrumentation of volatile somehow avoid
> > causing trouble?
>
> When used normally outside noinstr functions, because this is an
> __always_inline function, it will be instrumented. Within noinstr
> (which imply __no_kcsan) functions it should not be instrumented.
Got it, thank you!
This is going to require some serious commenting. ;-)
Thanx, Paul
> Thanks,
> -- Marco
>
>
> > > > follows:
> > > >
> > > > o rcu_nmi_exit(): After each following instrumentation_begin().
> > >
> > > Yes
> > >
> > > > o In theory in rcu_irq_exit_preempt(), but as this generates code
> > > > only in lockdep builds, it might not be worth worrying about.
> > > >
> > > > o Ditto for rcu_irq_exit_check_preempt().
> > > >
> > > > o Ditto for __rcu_irq_enter_check_tick().
> > >
> > > Not these, afaict they're all the above arch_atomic_read(), which is
> > > instrumented due to volatile in these cases.
> > >
> > > > o rcu_nmi_enter(): After each following instrumentation_begin().
> > >
> > > Yes
> > >
> > > > o __rcu_is_watching() is itself noinstr:
> > > >
> > > > o idtentry_enter_cond_rcu(): After each following
> > > > instrumentation_begin().
> > > >
> > > > o rcu_is_watching(): Either before or after the call to
> > > > rcu_dynticks_curr_cpu_in_eqs().
> > >
> > > Something like that yes.
> > >
> > > > > }
> > > > >
> > > > > /*
> > > > > @@ -692,6 +692,7 @@ noinstr void rcu_nmi_exit(void)
> > > > > {
> > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > >
> > > > > + instrumentation_begin();
> > > > > /*
> > > > > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > > > > * (We are exiting an NMI handler, so RCU better be paying attention
> > > > > @@ -705,7 +706,6 @@ noinstr void rcu_nmi_exit(void)
> > > > > * leave it in non-RCU-idle state.
> > > > > */
> > > > > if (rdp->dynticks_nmi_nesting != 1) {
> > > > > - instrumentation_begin();
> > > > > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > > > > atomic_read(&rdp->dynticks));
> > > > > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > > > > @@ -714,7 +714,6 @@ noinstr void rcu_nmi_exit(void)
> > > > > return;
> > > > > }
> > > > >
> > > > > - instrumentation_begin();
> > > > > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > > > > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > > > > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > > >
> > > > This one looks to be having no effect on instrumentation of atomics, but
> > > > rather coalescing a pair of instrumentation_begin() into one.
> > > >
> > > > Do I understand correctly?
> > >
> > > Almost, it puts the WARN_ON_ONCE()s under instrumentation_begin() too,
> > > and that makes a differnce, iirc it was the
> > > rcu_dynticks_curr_cpu_in_eqs() call that stood out. But that could've
> > > been before I switched it to arch_atomic_read(). In any case, I find
> > > this form a lot clearer.
> >
> > Got it, thank you.
> >
> > Thanx, Paul
On Thu, Jun 04, 2020 at 10:05:12AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 03, 2020 at 08:34:09PM -0700, Paul E. McKenney wrote:
> > On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> > > On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
>
> > > > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > > > {
> > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > >
> > > > > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > > > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > >
> > > The above is actually instrumented by KCSAN, due to arch_atomic_read()
> > > being a READ_ONCE() and it now understanding volatile.
> > >
> > > > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
> >
> > Right, this should instead be instrument_read(...).
> >
> > Though if KCSAN is unconditionally instrumenting volatile, how does
> > this help? Or does KCSAN's instrumentation of volatile somehow avoid
> > causing trouble?
>
> As Marco already explained, when used inside noinstr no instrumentation
> will be emitted, when used outside noinstr it will emit the right
> instrumentation.
>
> > > > o In theory in rcu_irq_exit_preempt(), but as this generates code
> > > > only in lockdep builds, it might not be worth worrying about.
> > > >
> > > > o Ditto for rcu_irq_exit_check_preempt().
> > > >
> > > > o Ditto for __rcu_irq_enter_check_tick().
> > >
> > > Not these, afaict they're all the above arch_atomic_read(), which is
> > > instrumented due to volatile in these cases.
>
> I this case, the above call-sites are all not noinstr (double negative!)
> and will thus cause instrumentation to be emitted.
>
> This is all a 'special' case for arch_atomic_read() (and _set()),
> because they're basically READ_ONCE() (and WRITE_ONCE() resp.). The
> normal atomics are asm() and it doesn't do anything for those (although
> I suppose clang could, since it has this internal assembler to parse the
> inline asm, but afaiu that's not something GCC ever wants to do).
Got it, and I had missed the inlining.
Again, commenting this will be interesting. And your earlier comment
about the compiler refusing to inline now makes sense...
Thanx, Paul
On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
> > // RCU is now watching. Better not be in an extended quiescent state!
> > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > !(seq & RCU_DYNTICK_CTRL_CTR));
> > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
>
> This one is gone in -rcu.
I'm still seeing that in mainline, was that removal scheduled for next
round?
> > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > }
> > }
What shall we do with this patch?
On Wed, Jun 03, 2020 at 01:40:16PM +0200, Peter Zijlstra wrote:
> A KCSAN build revealed we have explicit annoations through atomic_*()
> usage, switch to arch_atomic_*() for the respective functions.
>
> vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
How's this then? It anticipiates the removal of that andnot thing.
---
kernel/rcu/tree.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
* next idle sojourn.
*/
rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is no longer watching. Better be in extended quiescent state!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_CTR));
@@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
* and we also must force ordering with the next RCU read-side
* critical section.
*/
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is now watching. Better not be in an extended quiescent state!
rcu_dynticks_task_trace_exit(); // After ->dynticks update!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
!(seq & RCU_DYNTICK_CTRL_CTR));
if (seq & RCU_DYNTICK_CTRL_MASK) {
- atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
+ arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
smp_mb__after_atomic(); /* _exit after clearing mask. */
}
}
@@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
- return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
+ return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
}
/*
@@ -633,6 +633,10 @@ static noinstr void rcu_eqs_enter(bool u
do_nocb_deferred_wakeup(rdp);
rcu_prepare_for_idle();
rcu_preempt_deferred_qs(current);
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_enter()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
instrumentation_end();
WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid irq-access tearing. */
// RCU is watching here ...
@@ -692,6 +696,7 @@ noinstr void rcu_nmi_exit(void)
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
+ instrumentation_begin();
/*
* Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
* (We are exiting an NMI handler, so RCU better be paying attention
@@ -705,7 +710,6 @@ noinstr void rcu_nmi_exit(void)
* leave it in non-RCU-idle state.
*/
if (rdp->dynticks_nmi_nesting != 1) {
- instrumentation_begin();
trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
@@ -714,13 +718,15 @@ noinstr void rcu_nmi_exit(void)
return;
}
- instrumentation_begin();
/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
if (!in_nmi())
rcu_prepare_for_idle();
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_enter()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
instrumentation_end();
// RCU is watching here ...
@@ -838,6 +844,10 @@ static void noinstr rcu_eqs_exit(bool us
rcu_dynticks_eqs_exit();
// ... but is watching here.
instrumentation_begin();
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_exit()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
rcu_cleanup_after_idle();
trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, atomic_read(&rdp->dynticks));
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
@@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
if (!in_nmi())
rcu_cleanup_after_idle();
+ instrumentation_begin();
+ // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
+ instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
+ // instrumentation for the noinstr rcu_dynticks_eqs_exit()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
incby = 1;
} else if (!in_nmi()) {
instrumentation_begin();
rcu_irq_enter_check_tick();
- instrumentation_end();
}
- instrumentation_begin();
trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
rdp->dynticks_nmi_nesting,
rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
On Mon, Jun 15, 2020 at 05:30:52PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:
>
> > > // RCU is now watching. Better not be in an extended quiescent state!
> > > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > !(seq & RCU_DYNTICK_CTRL_CTR));
> > > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> >
> > This one is gone in -rcu.
>
> I'm still seeing that in mainline, was that removal scheduled for next
> round?
Yes. Unlike the few commits following it, this commit seems to work
fine even with the recent changes in mainline.
> > > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > > }
> > > }
>
> What shall we do with this patch?
I plan to submit it to the v5.9 merge window. Do you need it to get
to mainline earlier?
Thanx, Paul
On Mon, Jun 15, 2020 at 05:49:05PM +0200, Peter Zijlstra wrote:
> @@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
> if (!in_nmi())
> rcu_cleanup_after_idle();
>
> + instrumentation_begin();
> + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> +
> incby = 1;
> } else if (!in_nmi()) {
> instrumentation_begin();
> rcu_irq_enter_check_tick();
> - instrumentation_end();
> }
> - instrumentation_begin();
> trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> rdp->dynticks_nmi_nesting,
> rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
Oh, that's lost a possible instrumentation_begin() :/ But weirdly
objtool didn't complain about that... Let me poke at that.
On Mon, Jun 15, 2020 at 08:52:20AM -0700, Paul E. McKenney wrote:
> On Mon, Jun 15, 2020 at 05:30:52PM +0200, Peter Zijlstra wrote:
> > What shall we do with this patch?
>
> I plan to submit it to the v5.9 merge window. Do you need it to get
> to mainline earlier?
Yeah, we need it this round, to make KASAN/KCSAN work with the noinstr
stuff that just landed.
On Mon, Jun 15, 2020 at 05:55:13PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 05:49:05PM +0200, Peter Zijlstra wrote:
> > @@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
> > if (!in_nmi())
> > rcu_cleanup_after_idle();
> >
> > + instrumentation_begin();
> > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > incby = 1;
> > } else if (!in_nmi()) {
> > instrumentation_begin();
> > rcu_irq_enter_check_tick();
> > - instrumentation_end();
> > }
> > - instrumentation_begin();
> > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > rdp->dynticks_nmi_nesting,
> > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
>
> Oh, that's lost a possible instrumentation_begin() :/ But weirdly
> objtool didn't complain about that... Let me poke at that.
Like so then...
---
Subject: rcu: Fixup noinstr warnings
A KCSAN build revealed we have explicit annoations through atomic_*()
usage, switch to arch_atomic_*() for the respective functions.
vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
Additionally, without the NOP in instrumentation_begin(), objtool would
not detect the lack of the 'else instrumentation_begin();' branch in
rcu_nmi_enter().
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
include/linux/compiler.h | 2 +-
kernel/rcu/tree.c | 33 +++++++++++++++++++++++++--------
2 files changed, 26 insertions(+), 9 deletions(-)
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -123,7 +123,7 @@ void ftrace_likely_update(struct ftrace_
#ifdef CONFIG_DEBUG_ENTRY
/* Begin/end of an instrumentation safe region */
#define instrumentation_begin() ({ \
- asm volatile("%c0:\n\t" \
+ asm volatile("%c0: nop\n\t" \
".pushsection .discard.instr_begin\n\t" \
".long %c0b - .\n\t" \
".popsection\n\t" : : "i" (__COUNTER__)); \
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
* next idle sojourn.
*/
rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is no longer watching. Better be in extended quiescent state!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_CTR));
@@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
* and we also must force ordering with the next RCU read-side
* critical section.
*/
- seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
+ seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
// RCU is now watching. Better not be in an extended quiescent state!
rcu_dynticks_task_trace_exit(); // After ->dynticks update!
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
!(seq & RCU_DYNTICK_CTRL_CTR));
if (seq & RCU_DYNTICK_CTRL_MASK) {
- atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
+ arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
smp_mb__after_atomic(); /* _exit after clearing mask. */
}
}
@@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
- return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
+ return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
}
/*
@@ -633,6 +633,10 @@ static noinstr void rcu_eqs_enter(bool u
do_nocb_deferred_wakeup(rdp);
rcu_prepare_for_idle();
rcu_preempt_deferred_qs(current);
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_enter()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
instrumentation_end();
WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid irq-access tearing. */
// RCU is watching here ...
@@ -692,6 +696,7 @@ noinstr void rcu_nmi_exit(void)
{
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
+ instrumentation_begin();
/*
* Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
* (We are exiting an NMI handler, so RCU better be paying attention
@@ -705,7 +710,6 @@ noinstr void rcu_nmi_exit(void)
* leave it in non-RCU-idle state.
*/
if (rdp->dynticks_nmi_nesting != 1) {
- instrumentation_begin();
trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
@@ -714,13 +718,15 @@ noinstr void rcu_nmi_exit(void)
return;
}
- instrumentation_begin();
/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
if (!in_nmi())
rcu_prepare_for_idle();
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_enter()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
instrumentation_end();
// RCU is watching here ...
@@ -838,6 +844,10 @@ static void noinstr rcu_eqs_exit(bool us
rcu_dynticks_eqs_exit();
// ... but is watching here.
instrumentation_begin();
+
+ // instrumentation for the noinstr rcu_dynticks_eqs_exit()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
rcu_cleanup_after_idle();
trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, atomic_read(&rdp->dynticks));
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
@@ -983,13 +993,20 @@ noinstr void rcu_nmi_enter(void)
if (!in_nmi())
rcu_cleanup_after_idle();
+ instrumentation_begin();
+ // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
+ instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
+ // instrumentation for the noinstr rcu_dynticks_eqs_exit()
+ instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
+
incby = 1;
} else if (!in_nmi()) {
instrumentation_begin();
rcu_irq_enter_check_tick();
- instrumentation_end();
+ } else {
+ instrumentation_begin();
}
- instrumentation_begin();
+
trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
rdp->dynticks_nmi_nesting,
rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
On Mon, Jun 15, 2020 at 06:24:27PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 05:55:13PM +0200, Peter Zijlstra wrote:
> > On Mon, Jun 15, 2020 at 05:49:05PM +0200, Peter Zijlstra wrote:
> > > @@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
> > > if (!in_nmi())
> > > rcu_cleanup_after_idle();
> > >
> > > + instrumentation_begin();
> > > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > > +
> > > incby = 1;
> > > } else if (!in_nmi()) {
> > > instrumentation_begin();
> > > rcu_irq_enter_check_tick();
> > > - instrumentation_end();
> > > }
> > > - instrumentation_begin();
> > > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > > rdp->dynticks_nmi_nesting,
> > > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
> >
> > Oh, that's lost a possible instrumentation_begin() :/ But weirdly
> > objtool didn't complain about that... Let me poke at that.
This merge window has been quite the trainwreck, hasn't it? :-/
> Like so then...
Looks plausible, firing up some tests.
Thanx, Paul
> ---
> Subject: rcu: Fixup noinstr warnings
>
> A KCSAN build revealed we have explicit annoations through atomic_*()
> usage, switch to arch_atomic_*() for the respective functions.
>
> vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
> vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
>
> Additionally, without the NOP in instrumentation_begin(), objtool would
> not detect the lack of the 'else instrumentation_begin();' branch in
> rcu_nmi_enter().
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/compiler.h | 2 +-
> kernel/rcu/tree.c | 33 +++++++++++++++++++++++++--------
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -123,7 +123,7 @@ void ftrace_likely_update(struct ftrace_
> #ifdef CONFIG_DEBUG_ENTRY
> /* Begin/end of an instrumentation safe region */
> #define instrumentation_begin() ({ \
> - asm volatile("%c0:\n\t" \
> + asm volatile("%c0: nop\n\t" \
> ".pushsection .discard.instr_begin\n\t" \
> ".long %c0b - .\n\t" \
> ".popsection\n\t" : : "i" (__COUNTER__)); \
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> * next idle sojourn.
> */
> rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> // RCU is no longer watching. Better be in extended quiescent state!
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> (seq & RCU_DYNTICK_CTRL_CTR));
> @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> * and we also must force ordering with the next RCU read-side
> * critical section.
> */
> - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> // RCU is now watching. Better not be in an extended quiescent state!
> rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> !(seq & RCU_DYNTICK_CTRL_CTR));
> if (seq & RCU_DYNTICK_CTRL_MASK) {
> - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> smp_mb__after_atomic(); /* _exit after clearing mask. */
> }
> }
> @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> }
>
> /*
> @@ -633,6 +633,10 @@ static noinstr void rcu_eqs_enter(bool u
> do_nocb_deferred_wakeup(rdp);
> rcu_prepare_for_idle();
> rcu_preempt_deferred_qs(current);
> +
> + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> +
> instrumentation_end();
> WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid irq-access tearing. */
> // RCU is watching here ...
> @@ -692,6 +696,7 @@ noinstr void rcu_nmi_exit(void)
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> + instrumentation_begin();
> /*
> * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> * (We are exiting an NMI handler, so RCU better be paying attention
> @@ -705,7 +710,6 @@ noinstr void rcu_nmi_exit(void)
> * leave it in non-RCU-idle state.
> */
> if (rdp->dynticks_nmi_nesting != 1) {
> - instrumentation_begin();
> trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> atomic_read(&rdp->dynticks));
> WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> @@ -714,13 +718,15 @@ noinstr void rcu_nmi_exit(void)
> return;
> }
>
> - instrumentation_begin();
> /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
>
> if (!in_nmi())
> rcu_prepare_for_idle();
> +
> + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> instrumentation_end();
>
> // RCU is watching here ...
> @@ -838,6 +844,10 @@ static void noinstr rcu_eqs_exit(bool us
> rcu_dynticks_eqs_exit();
> // ... but is watching here.
> instrumentation_begin();
> +
> + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> +
> rcu_cleanup_after_idle();
> trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, atomic_read(&rdp->dynticks));
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> @@ -983,13 +993,20 @@ noinstr void rcu_nmi_enter(void)
> if (!in_nmi())
> rcu_cleanup_after_idle();
>
> + instrumentation_begin();
> + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> +
> incby = 1;
> } else if (!in_nmi()) {
> instrumentation_begin();
> rcu_irq_enter_check_tick();
> - instrumentation_end();
> + } else {
> + instrumentation_begin();
> }
> - instrumentation_begin();
> +
> trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> rdp->dynticks_nmi_nesting,
> rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
On Mon, Jun 15, 2020 at 10:14:04AM -0700, Paul E. McKenney wrote:
> This merge window has been quite the trainwreck, hasn't it? :-/
Keeps life interesting I suppose..
On Mon, Jun 15, 2020 at 08:33:25PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 10:14:04AM -0700, Paul E. McKenney wrote:
>
> > This merge window has been quite the trainwreck, hasn't it? :-/
>
> Keeps life interesting I suppose..
;-) ;-) ;-)
Thanx, Paul
On Mon, Jun 15, 2020 at 10:14:04AM -0700, Paul E. McKenney wrote:
> On Mon, Jun 15, 2020 at 06:24:27PM +0200, Peter Zijlstra wrote:
> > On Mon, Jun 15, 2020 at 05:55:13PM +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 15, 2020 at 05:49:05PM +0200, Peter Zijlstra wrote:
> > > > @@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
> > > > if (!in_nmi())
> > > > rcu_cleanup_after_idle();
> > > >
> > > > + instrumentation_begin();
> > > > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > > > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > > > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > > > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > > > +
> > > > incby = 1;
> > > > } else if (!in_nmi()) {
> > > > instrumentation_begin();
> > > > rcu_irq_enter_check_tick();
> > > > - instrumentation_end();
> > > > }
> > > > - instrumentation_begin();
> > > > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > > > rdp->dynticks_nmi_nesting,
> > > > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
> > >
> > > Oh, that's lost a possible instrumentation_begin() :/ But weirdly
> > > objtool didn't complain about that... Let me poke at that.
>
> This merge window has been quite the trainwreck, hasn't it? :-/
>
> > Like so then...
>
> Looks plausible, firing up some tests.
And it passes light rcutorture testing across all the scenarios.
So looks even more plausible. ;-)
Thanx, Paul
> > ---
> > Subject: rcu: Fixup noinstr warnings
> >
> > A KCSAN build revealed we have explicit annoations through atomic_*()
> > usage, switch to arch_atomic_*() for the respective functions.
> >
> > vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> >
> > Additionally, without the NOP in instrumentation_begin(), objtool would
> > not detect the lack of the 'else instrumentation_begin();' branch in
> > rcu_nmi_enter().
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > include/linux/compiler.h | 2 +-
> > kernel/rcu/tree.c | 33 +++++++++++++++++++++++++--------
> > 2 files changed, 26 insertions(+), 9 deletions(-)
> >
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -123,7 +123,7 @@ void ftrace_likely_update(struct ftrace_
> > #ifdef CONFIG_DEBUG_ENTRY
> > /* Begin/end of an instrumentation safe region */
> > #define instrumentation_begin() ({ \
> > - asm volatile("%c0:\n\t" \
> > + asm volatile("%c0: nop\n\t" \
> > ".pushsection .discard.instr_begin\n\t" \
> > ".long %c0b - .\n\t" \
> > ".popsection\n\t" : : "i" (__COUNTER__)); \
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > * next idle sojourn.
> > */
> > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > // RCU is no longer watching. Better be in extended quiescent state!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > (seq & RCU_DYNTICK_CTRL_CTR));
> > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > * and we also must force ordering with the next RCU read-side
> > * critical section.
> > */
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > // RCU is now watching. Better not be in an extended quiescent state!
> > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > !(seq & RCU_DYNTICK_CTRL_CTR));
> > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > }
> > }
> > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > }
> >
> > /*
> > @@ -633,6 +633,10 @@ static noinstr void rcu_eqs_enter(bool u
> > do_nocb_deferred_wakeup(rdp);
> > rcu_prepare_for_idle();
> > rcu_preempt_deferred_qs(current);
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > instrumentation_end();
> > WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid irq-access tearing. */
> > // RCU is watching here ...
> > @@ -692,6 +696,7 @@ noinstr void rcu_nmi_exit(void)
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > + instrumentation_begin();
> > /*
> > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > * (We are exiting an NMI handler, so RCU better be paying attention
> > @@ -705,7 +710,6 @@ noinstr void rcu_nmi_exit(void)
> > * leave it in non-RCU-idle state.
> > */
> > if (rdp->dynticks_nmi_nesting != 1) {
> > - instrumentation_begin();
> > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > @@ -714,13 +718,15 @@ noinstr void rcu_nmi_exit(void)
> > return;
> > }
> >
> > - instrumentation_begin();
> > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> >
> > if (!in_nmi())
> > rcu_prepare_for_idle();
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > instrumentation_end();
> >
> > // RCU is watching here ...
> > @@ -838,6 +844,10 @@ static void noinstr rcu_eqs_exit(bool us
> > rcu_dynticks_eqs_exit();
> > // ... but is watching here.
> > instrumentation_begin();
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > rcu_cleanup_after_idle();
> > trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, atomic_read(&rdp->dynticks));
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> > @@ -983,13 +993,20 @@ noinstr void rcu_nmi_enter(void)
> > if (!in_nmi())
> > rcu_cleanup_after_idle();
> >
> > + instrumentation_begin();
> > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > incby = 1;
> > } else if (!in_nmi()) {
> > instrumentation_begin();
> > rcu_irq_enter_check_tick();
> > - instrumentation_end();
> > + } else {
> > + instrumentation_begin();
> > }
> > - instrumentation_begin();
> > +
> > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > rdp->dynticks_nmi_nesting,
> > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
On Mon, Jun 15, 2020 at 10:14:04AM -0700, Paul E. McKenney wrote:
> On Mon, Jun 15, 2020 at 06:24:27PM +0200, Peter Zijlstra wrote:
> > On Mon, Jun 15, 2020 at 05:55:13PM +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 15, 2020 at 05:49:05PM +0200, Peter Zijlstra wrote:
> > > > @@ -983,13 +993,17 @@ noinstr void rcu_nmi_enter(void)
> > > > if (!in_nmi())
> > > > rcu_cleanup_after_idle();
> > > >
> > > > + instrumentation_begin();
> > > > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > > > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > > > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > > > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > > > +
> > > > incby = 1;
> > > > } else if (!in_nmi()) {
> > > > instrumentation_begin();
> > > > rcu_irq_enter_check_tick();
> > > > - instrumentation_end();
> > > > }
> > > > - instrumentation_begin();
> > > > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > > > rdp->dynticks_nmi_nesting,
> > > > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
> > >
> > > Oh, that's lost a possible instrumentation_begin() :/ But weirdly
> > > objtool didn't complain about that... Let me poke at that.
>
> This merge window has been quite the trainwreck, hasn't it? :-/
>
> > Like so then...
>
> Looks plausible, firing up some tests.
Just following up because I don't see this anywhere. If I am supposed
to take this (which is more plausible now that v5.8-rc1 is out), please
let me know.
Thanx, Paul
> > ---
> > Subject: rcu: Fixup noinstr warnings
> >
> > A KCSAN build revealed we have explicit annoations through atomic_*()
> > usage, switch to arch_atomic_*() for the respective functions.
> >
> > vmlinux.o: warning: objtool: rcu_nmi_exit()+0x4d: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_dynticks_eqs_enter()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_nmi_enter()+0x4f: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: rcu_dynticks_eqs_exit()+0x2a: call to __kcsan_check_access() leaves .noinstr.text section
> > vmlinux.o: warning: objtool: __rcu_is_watching()+0x25: call to __kcsan_check_access() leaves .noinstr.text section
> >
> > Additionally, without the NOP in instrumentation_begin(), objtool would
> > not detect the lack of the 'else instrumentation_begin();' branch in
> > rcu_nmi_enter().
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > include/linux/compiler.h | 2 +-
> > kernel/rcu/tree.c | 33 +++++++++++++++++++++++++--------
> > 2 files changed, 26 insertions(+), 9 deletions(-)
> >
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -123,7 +123,7 @@ void ftrace_likely_update(struct ftrace_
> > #ifdef CONFIG_DEBUG_ENTRY
> > /* Begin/end of an instrumentation safe region */
> > #define instrumentation_begin() ({ \
> > - asm volatile("%c0:\n\t" \
> > + asm volatile("%c0: nop\n\t" \
> > ".pushsection .discard.instr_begin\n\t" \
> > ".long %c0b - .\n\t" \
> > ".popsection\n\t" : : "i" (__COUNTER__)); \
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -250,7 +250,7 @@ static noinstr void rcu_dynticks_eqs_ent
> > * next idle sojourn.
> > */
> > rcu_dynticks_task_trace_enter(); // Before ->dynticks update!
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > // RCU is no longer watching. Better be in extended quiescent state!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > (seq & RCU_DYNTICK_CTRL_CTR));
> > @@ -274,13 +274,13 @@ static noinstr void rcu_dynticks_eqs_exi
> > * and we also must force ordering with the next RCU read-side
> > * critical section.
> > */
> > - seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > + seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > // RCU is now watching. Better not be in an extended quiescent state!
> > rcu_dynticks_task_trace_exit(); // After ->dynticks update!
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > !(seq & RCU_DYNTICK_CTRL_CTR));
> > if (seq & RCU_DYNTICK_CTRL_MASK) {
> > - atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > + arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > smp_mb__after_atomic(); /* _exit after clearing mask. */
> > }
> > }
> > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > - return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > + return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > }
> >
> > /*
> > @@ -633,6 +633,10 @@ static noinstr void rcu_eqs_enter(bool u
> > do_nocb_deferred_wakeup(rdp);
> > rcu_prepare_for_idle();
> > rcu_preempt_deferred_qs(current);
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > instrumentation_end();
> > WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid irq-access tearing. */
> > // RCU is watching here ...
> > @@ -692,6 +696,7 @@ noinstr void rcu_nmi_exit(void)
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > + instrumentation_begin();
> > /*
> > * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> > * (We are exiting an NMI handler, so RCU better be paying attention
> > @@ -705,7 +710,6 @@ noinstr void rcu_nmi_exit(void)
> > * leave it in non-RCU-idle state.
> > */
> > if (rdp->dynticks_nmi_nesting != 1) {
> > - instrumentation_begin();
> > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> > atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > @@ -714,13 +718,15 @@ noinstr void rcu_nmi_exit(void)
> > return;
> > }
> >
> > - instrumentation_begin();
> > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> >
> > if (!in_nmi())
> > rcu_prepare_for_idle();
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_enter()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > instrumentation_end();
> >
> > // RCU is watching here ...
> > @@ -838,6 +844,10 @@ static void noinstr rcu_eqs_exit(bool us
> > rcu_dynticks_eqs_exit();
> > // ... but is watching here.
> > instrumentation_begin();
> > +
> > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > rcu_cleanup_after_idle();
> > trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, atomic_read(&rdp->dynticks));
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> > @@ -983,13 +993,20 @@ noinstr void rcu_nmi_enter(void)
> > if (!in_nmi())
> > rcu_cleanup_after_idle();
> >
> > + instrumentation_begin();
> > + // instrumentation for the noinstr rcu_dynticks_curr_cpu_in_eqs()
> > + instrument_atomic_read(&rdp->dynticks, sizeof(rdp->dynticks));
> > + // instrumentation for the noinstr rcu_dynticks_eqs_exit()
> > + instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks));
> > +
> > incby = 1;
> > } else if (!in_nmi()) {
> > instrumentation_begin();
> > rcu_irq_enter_check_tick();
> > - instrumentation_end();
> > + } else {
> > + instrumentation_begin();
> > }
> > - instrumentation_begin();
> > +
> > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> > rdp->dynticks_nmi_nesting,
> > rdp->dynticks_nmi_nesting + incby, atomic_read(&rdp->dynticks));
On Fri, Jun 19, 2020 at 03:15:55PM -0700, Paul E. McKenney wrote:
> Just following up because I don't see this anywhere. If I am supposed
> to take this (which is more plausible now that v5.8-rc1 is out), please
> let me know.
Sorry, I got distracted by that NULL ptr thing, but that seems sorted
now. If you don't mind taking it through your rcu/urgent tree for -rc3
or so that would be awesome.
On Tue, Jun 23, 2020 at 10:46:46PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 19, 2020 at 03:15:55PM -0700, Paul E. McKenney wrote:
>
> > Just following up because I don't see this anywhere. If I am supposed
> > to take this (which is more plausible now that v5.8-rc1 is out), please
> > let me know.
>
> Sorry, I got distracted by that NULL ptr thing, but that seems sorted
> now. If you don't mind taking it through your rcu/urgent tree for -rc3
> or so that would be awesome.
Will do!
Just to double-check, this is the patch from you with Message-ID
[email protected], correct?
Or, if you prefer, this commit now on -rcu?
5fe289eccfe5 ("rcu: Fixup noinstr warnings")
If this is the correct commit, I will rebase it on top of v5.8-rc2,
and if it passes tests, send it along via rcu/urgent.
Thanx, Paul
On Tue, Jun 23, 2020 at 02:44:33PM -0700, Paul E. McKenney wrote:
> On Tue, Jun 23, 2020 at 10:46:46PM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 19, 2020 at 03:15:55PM -0700, Paul E. McKenney wrote:
> >
> > > Just following up because I don't see this anywhere. If I am supposed
> > > to take this (which is more plausible now that v5.8-rc1 is out), please
> > > let me know.
> >
> > Sorry, I got distracted by that NULL ptr thing, but that seems sorted
> > now. If you don't mind taking it through your rcu/urgent tree for -rc3
> > or so that would be awesome.
>
> Will do!
>
> Just to double-check, this is the patch from you with Message-ID
> [email protected], correct?
>
> Or, if you prefer, this commit now on -rcu?
>
> 5fe289eccfe5 ("rcu: Fixup noinstr warnings")
>
> If this is the correct commit, I will rebase it on top of v5.8-rc2,
> and if it passes tests, send it along via rcu/urgent.
Ah, I was thinking about:
https://lore.kernel.org/lkml/[email protected]/
seeing how I added that instrumentation you wanted :-), but either
version should work for now. KCSAN is sad without this.
On Wed, Jun 24, 2020 at 09:52:49AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 23, 2020 at 02:44:33PM -0700, Paul E. McKenney wrote:
> > On Tue, Jun 23, 2020 at 10:46:46PM +0200, Peter Zijlstra wrote:
> > > On Fri, Jun 19, 2020 at 03:15:55PM -0700, Paul E. McKenney wrote:
> > >
> > > > Just following up because I don't see this anywhere. If I am supposed
> > > > to take this (which is more plausible now that v5.8-rc1 is out), please
> > > > let me know.
> > >
> > > Sorry, I got distracted by that NULL ptr thing, but that seems sorted
> > > now. If you don't mind taking it through your rcu/urgent tree for -rc3
> > > or so that would be awesome.
> >
> > Will do!
> >
> > Just to double-check, this is the patch from you with Message-ID
> > [email protected], correct?
> >
> > Or, if you prefer, this commit now on -rcu?
> >
> > 5fe289eccfe5 ("rcu: Fixup noinstr warnings")
> >
> > If this is the correct commit, I will rebase it on top of v5.8-rc2,
> > and if it passes tests, send it along via rcu/urgent.
>
> Ah, I was thinking about:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> seeing how I added that instrumentation you wanted :-), but either
> version should work for now. KCSAN is sad without this.
Glad I asked! I will substitute the one you pointed out above.
Thanx, Paul