"Paul E. McKenney" <paulmck@xxxxxxxxxx> writes:
> On Fri, Mar 20, 2020 at 07:00:13PM +0100, Thomas Gleixner wrote:
>> -void rcu_user_enter(void)
>> +noinstr void rcu_user_enter(void)
>> {
>> lockdep_assert_irqs_disabled();
>
> Just out of curiosity -- this means that lockdep_assert_irqs_disabled()
> must be noinstr, correct?
Yes. noinstr functions can call other noinstr functions safely. If there
is a instr_begin() then anything can be called up to the corresponding
instr_end(). After that the noinstr rule applies again.
>> if (rdp->dynticks_nmi_nesting != 1) {
>> + instr_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. */
>> rdp->dynticks_nmi_nesting - 2);
>> + instr_end();
>> return;
>> }
>>
>> + instr_begin();
>
> Indentation?
Is obviously wrong. You found it so please keep the extra TAB for times
when you need a spare one :)
>> * If you add or remove a call to rcu_user_exit(), be sure to test with
>> * CONFIG_RCU_EQS_DEBUG=y.
>> */
>> -void rcu_user_exit(void)
>> +void noinstr rcu_user_exit(void)
>> {
>> rcu_eqs_exit(1);
>> }
>> @@ -830,27 +833,33 @@ static __always_inline void rcu_nmi_ente
>> rcu_cleanup_after_idle();
>>
>> incby = 1;
>> - } else if (irq && tick_nohz_full_cpu(rdp->cpu) &&
>> - rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
>> - READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
>> + } else if (irq) {
>> // We get here only if we had already exited the extended
>> // quiescent state and this was an interrupt (not an NMI).
>> // Therefore, (1) RCU is already watching and (2) The fact
>> // that we are in an interrupt handler and that the rcu_node
>> // lock is an irq-disabled lock prevents self-deadlock.
>> // So we can safely recheck under the lock.
>
> The above comment is a bit misleading in this location.
True
>> - raw_spin_lock_rcu_node(rdp->mynode);
>> - if (rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
>> - // A nohz_full CPU is in the kernel and RCU
>> - // needs a quiescent state. Turn on the tick!
>> - rdp->rcu_forced_tick = true;
>> - tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
>> + instr_begin();
>> + if (tick_nohz_full_cpu(rdp->cpu) &&
>> + rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
>> + READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
>
> So how about like this?
>
> // We get here only if we had already exited
> // the extended quiescent state and this was an
> // interrupt (not an NMI). Therefore, (1) RCU is
> // already watching and (2) The fact that we are in
> // an interrupt handler and that the rcu_node lock
> // is an irq-disabled lock prevents self-deadlock.
> // So we can safely recheck under the lock.
Yup
Thanks,
tglx
