Re: [patch V9 02/39] rcu: Abstract out rcu_irq_enter_check_tick() from rcu_nmi_enter()

["Paul E. McKenney" <paulmck@kernel.org>]

On Tue, May 26, 2020 at 10:14:56AM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> > > +	if (!tick_nohz_full_cpu(rdp->cpu) ||
> > > +	    !READ_ONCE(rdp->rcu_urgent_qs) ||
> > > +	    READ_ONCE(rdp->rcu_forced_tick)) {
> > > +		// RCU doesn't need nohz_full help from this CPU, or it is
> > > +		// already getting that help.
> > > +		return;
> > > +	}
> > > +
> > > +	// We get here only when not in an extended quiescent state and
> > > +	// from interrupts (as opposed to NMIs).  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.
> > > +	// Note that the nohz_full state currently cannot change.
> > > +	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!
> > > +		WRITE_ONCE(rdp->rcu_forced_tick, true);
> > > +		tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
> > > +	}
> > > +	raw_spin_unlock_rcu_node(rdp->mynode);
> 
> BTW., can we please not ever use this weird comment style in the future?
> 
> Linus gave an exception to single-line C++ style comments - but I 
> don't think that should be extrapolated to a license to uglify the 
> kernel with inconsistent muck like this. :-/
> 
> I've sanitized it via the patch below.

The "//" comment style does save vertical space.  Is it really ugly
or just unfamiliar?  For purposes of comparison, back in the day, the
"/* */" style seemed quite strange compared to my earlier languages'
commenting styles.

> ( I also fixed the whitespace damage and a capitalization typo while 
>   at it, and fixed the spelling in the big comment explaining 
>   __rcu_irq_enter_check_tick(). )

Some were stylistic rather than wrong, but I have no objection to
any of these changes.

							Thanx, Paul

> Thanks,
> 
> 	Ingo
> 
> --- tip.orig/kernel/rcu/tree.c
> +++ tip/kernel/rcu/tree.c
> @@ -850,14 +850,14 @@ void noinstr rcu_user_exit(void)
>  }
>  
>  /**
> - * __rcu_irq_enter_check_tick - Enable scheduler tick on CPU if RCU needs it.
> + * __rcu_irq_enter_check_tick - Enable the scheduler tick on a CPU if RCU needs it.
>   *
>   * The scheduler tick is not normally enabled when CPUs enter the kernel
>   * from nohz_full userspace execution.  After all, nohz_full userspace
>   * execution is an RCU quiescent state and the time executing in the kernel
> - * is quite short.  Except of course when it isn't.  And it is not hard to
> + * is quite short.  Except of course when it isn't: it is not hard to
>   * cause a large system to spend tens of seconds or even minutes looping
> - * in the kernel, which can cause a number of problems, include RCU CPU
> + * in the kernel, which can cause a number of problems, including RCU CPU
>   * stall warnings.
>   *
>   * Therefore, if a nohz_full CPU fails to report a quiescent state
> @@ -879,7 +879,7 @@ void __rcu_irq_enter_check_tick(void)
>  {
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  
> -	 // Enabling the tick is unsafe in NMI handlers.
> +	/* Enabling the tick is unsafe in NMI handlers. */
>  	if (WARN_ON_ONCE(in_nmi()))
>  		return;
>  
> @@ -889,21 +889,27 @@ void __rcu_irq_enter_check_tick(void)
>  	if (!tick_nohz_full_cpu(rdp->cpu) ||
>  	    !READ_ONCE(rdp->rcu_urgent_qs) ||
>  	    READ_ONCE(rdp->rcu_forced_tick)) {
> -		// RCU doesn't need nohz_full help from this CPU, or it is
> -		// already getting that help.
> +		/*
> +		 * RCU doesn't need nohz_full help from this CPU, or it is
> +		 * already getting that help.
> +		 */
>  		return;
>  	}
>  
> -	// We get here only when not in an extended quiescent state and
> -	// from interrupts (as opposed to NMIs).  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.
> -	// Note that the nohz_full state currently cannot change.
> +	/*
> +	 * We get here only when not in an extended quiescent state and
> +	 * from interrupts (as opposed to NMIs).  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.
> +	 * Note that the nohz_full state currently cannot change.
> +	 */
>  	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!
> +		/*
> +		 * A nohz_full CPU is in the kernel and RCU needs a
> +		 * quiescent state.  Turn on the tick!
> +		 */
>  		WRITE_ONCE(rdp->rcu_forced_tick, true);
>  		tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
>  	}