Re: need_heavy_qs flag for PREEMPT=y kernels

[Joel Fernandes <joel@joelfernandes.org>]

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.

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;
-- 
2.23.0.rc1.153.gdeed80330f-goog