lkml.org 
[lkml]   [2020]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch V9 02/39] rcu: Abstract out rcu_irq_enter_check_tick() from rcu_nmi_enter()
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);
> }

\
 
 \ /
  Last update: 2020-05-26 17:35    [W:0.097 / U:1.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site