On Thu, Aug 15, 2019 at 05:22:16PM -0400, Joel Fernandes wrote:
> On Thu, Aug 15, 2019 at 01:31:07PM -0700, Paul E. McKenney wrote:
> > On Thu, Aug 15, 2019 at 04:04:32PM -0400, Joel Fernandes wrote:
> > > On Thu, Aug 15, 2019 at 10:17:14AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Aug 12, 2019 at 09:02:49PM -0400, Joel Fernandes wrote:
> > > > > On Mon, Aug 12, 2019 at 04:01:38PM -0700, Paul E. McKenney wrote:
> > > >
> > > > [ . . . ]
> > > >
> > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > index 8c494a692728..ad906d6a74fb 100644
> > > > > > --- a/kernel/rcu/tree.c
> > > > > > +++ b/kernel/rcu/tree.c
> > > > > > @@ -651,6 +651,12 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> > > > > > */
> > > > > > if (rdp->dynticks_nmi_nesting != 1) {
> > > > > > trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2, rdp->dynticks);
> > > > > > + if (tick_nohz_full_cpu(rdp->cpu) &&
> > > > > > + rdp->dynticks_nmi_nesting == 2 &&
> > > > > > + rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
> > > > > > + rdp->rcu_forced_tick = true;
> > > > > > + tick_dep_set_cpu(rdp->cpu, TICK_DEP_MASK_RCU);
> > > > > > + }
> > > > >
> > > > >
> > > > > Instead of checking dynticks_nmi_nesting == 2 in rcu_nmi_exit_common(), can
> > > > > we do the tick_dep_set_cpu(rdp->cpu, TICK_DEP_MASK_RCU) from
> > > > > rcu_nmi_enter_common() ? We could add this code there, under the "if
> > > > > (rcu_dynticks_curr_cpu_in_eqs())".
> > > >
> > > > This would need to go in an "else" clause, correct? But there would still
> > > > want to be a check for interrupt from base level (which would admittedly
> > > > be an equality comparison with zero) and we would also still need to check
> > > > for rdp->rcu_urgent_qs && !rdp->rcu_forced_tick.
> > >
> > > True, agreed. I replied to this before saying it should be
> > > !rcu_dynticks_curr_cpu_in_eqs() in the "if" ;) But it seems I could also be
> > > missing the check for TICK_DEP_MASK_RCU in my tree so I think we need this as
> > > well which is below as diff. Testing it more now!
> > >
> > > And, with this I do get many more ticks during the test. But there are
> > > intervals where the tick is not seen. Still it is much better than before:
> > >
> > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > > index be9707f68024..e697c7a2ce67 100644
> > > --- a/kernel/time/tick-sched.c
> > > +++ b/kernel/time/tick-sched.c
> > > @@ -198,6 +198,10 @@ static bool check_tick_dependency(atomic_t *dep)
> > > return true;
> > > }
> > >
> > > + if (val & TICK_DEP_MASK_CLOCK_RCU) {
> > > + return true;
> > > + }
> > > +
> > > return false;
> > > }
> >
> > That one is not in my tree, either. Frederic, should I add this to
> > your patch? For that matter, may I add your Signed-off-by as well?
> > Your original is in my -rcu tree at:
> >
> > 0cb41806c799 ("EXP nohz: Add TICK_DEP_BIT_RCU")
> >
> > I am testing Joel's suggested addition now.
>
> Actually there's more addition needed! I found another thing missing:
>
> There's a per-cpu &tick_dep_mask and a per-cpu ts->tick_dep_mask. It seems
> RCU is setting the latter.
As I understand it, tick_dep_mask forces the tick on globally,
ts->tick_dep_mask forces it on for a specific CPU (which RCU uses when it
needs a quiescent state from that CPU), current->tick_dep_mask forces
it on for a specific task (which RCU uses for callback invocation
and certain rcutorture kthreads), and I don't pretend to understand
current->signal->tick_dep_mask (the comment says something about POSIX
CPU timers).
But it looks to me that can_stop_full_tick() and check_tick_dependency()
already cover all of these. What am I missing?
> So I added a check for both, below is the diff:
>
> However, I see in some cases that the tick_dep_mask is just 0 but I have to
> debug that tomorrow if that's an issue on the RCU side of things. For now,
> below should be the completed Frederick patch which you could squash into his
> if he's Ok with it:
>
> ---8<-----------------------
>
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index f92a10b5e112..3f476e2a4bf7 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -108,7 +108,8 @@ enum tick_dep_bits {
> TICK_DEP_BIT_POSIX_TIMER = 0,
> TICK_DEP_BIT_PERF_EVENTS = 1,
> TICK_DEP_BIT_SCHED = 2,
> - TICK_DEP_BIT_CLOCK_UNSTABLE = 3
> + TICK_DEP_BIT_CLOCK_UNSTABLE = 3,
> + TICK_DEP_BIT_RCU = 4
> };
>
> #define TICK_DEP_MASK_NONE 0
> @@ -116,6 +117,7 @@ enum tick_dep_bits {
> #define TICK_DEP_MASK_PERF_EVENTS (1 << TICK_DEP_BIT_PERF_EVENTS)
> #define TICK_DEP_MASK_SCHED (1 << TICK_DEP_BIT_SCHED)
> #define TICK_DEP_MASK_CLOCK_UNSTABLE (1 << TICK_DEP_BIT_CLOCK_UNSTABLE)
> +#define TICK_DEP_MASK_RCU (1 << TICK_DEP_BIT_RCU)
>
> #ifdef CONFIG_NO_HZ_COMMON
> extern bool tick_nohz_enabled;
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index be9707f68024..a613916cc3f0 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -198,6 +198,11 @@ static bool check_tick_dependency(atomic_t *dep)
> return true;
> }
>
> + if (val & TICK_DEP_MASK_RCU) {
> + trace_tick_stop(0, TICK_DEP_MASK_RCU);
> + return true;
> + }
> +
> return false;
> }
>
> @@ -208,8 +213,13 @@ static bool can_stop_full_tick(int cpu, struct tick_sched *ts)
> if (unlikely(!cpu_online(cpu)))
> return false;
>
> - if (check_tick_dependency(&tick_dep_mask))
> + if (check_tick_dependency(&ts->tick_dep_mask)) {
> return false;
> + }
> +
> + if (check_tick_dependency(&tick_dep_mask)) {
> + return false;
> + }
>
> if (check_tick_dependency(&ts->tick_dep_mask))
> return false;
You lost me on this one. How does it help to check ts->tick_dep_mask
twice? And why is it important to check it before checking tick_dep_mask,
especially given that the common case of all-zero masks will cause
all to be checked anyway?
Thanx, Paul
