Re: linux-next-20110923: warning kernel/rcutree.c:1833

[Paul E. McKenney]

On Mon, Oct 03, 2011 at 09:30:36AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 03, 2011 at 03:03:48PM +0200, Frederic Weisbecker wrote:
> > On Sun, Oct 02, 2011 at 05:32:47PM -0700, Paul E. McKenney wrote:
> > > > > -void rcu_irq_enter(void)
> > > > > +int rcu_is_cpu_idle(void)
> > > > >  {
> > > > > -	rcu_exit_nohz();
> > > > > +	return (atomic_read(&__get_cpu_var(rcu_dynticks).dynticks) & 0x1) == 0;
> > > > >  }
> > > > 
> > > > So that's not used in this patch but it's interesting for me
> > > > to backport "rcu: Detect illegal rcu dereference in extended quiescent state".
> > > 
> > > Yep, that is why it is there.
> > 
> > Ok.
> > 
> > > 
> > > > The above should be read from a preempt disabled section though
> > > > (remember "rcu: Fix preempt-unsafe debug check of rcu extended quiescent state")
> > > 
> > > Yes, and that is why the last line of the header comment reads "The
> > > caller must have at least disabled preemption."  Disabling preemption
> > > is not necessary in Tiny RCU because there is no other CPU for the task
> > > to go to.  (Right?)
> > 
> > Right.
> > 
> > > > Those functions should probably lay in a separate patch. But I don't mind
> > > > much keeping the things as is and use these APIs in my next patches though.
> > > > I'll just fix the preempt enabled thing above.
> > > 
> > > Or were you saying that you wish to make calls to rcu_is_cpu_idle()
> > > that have preemption enabled?
> > 
> > Yeah. That's going to be called from places like rcu_read_lock_held()
> > and things like this that don't need to disable preemption themselves.
> > 
> > Would be better to disable preemption from that function.
> 
> Hmmm...  This might be a good use for the "drive-by" per-CPU access
> functions.
> 
> No, that doesn't work.  We could pick up the pointer, switch to another
> CPU, the original CPU could run a task that blocks before we start running,
> and then we could incorrectly decide that we were running in idle context,
> issuing a spurious warning.  This approach would only work in environments
> that (unlike the Linux kernel) mapped all the per-CPU variables to the
> same virtual address on all CPUs.  (DYNIX/ptx did this, but this leads
> to other problems, like being unable to reasonably access other CPUs'
> variables.  Double mapping has other issues on some architectures.)
> 
> OK, agreed.  I will make this function disable preemption.
> 
> > > And I can split the patch easily enough while keeping the diff the same,
> > > so you should be able to do your porting on top of the existing code.
> > 
> > No I'm actually pretty fine with the current state. Whether that's defined
> > in this patch or a following one is actually not important.
> 
> Fair enough!

And here is an update that might handle an irq entry/exit miscounting
problem.  Thanks to Arjan van de Ven for pointing out that my earlier
approach would in fact miscount irq entries/exits in face of things like
upcalls to user-mode helpers.

This is experimental, and might well hurt more than it helps.  Testing
ongoing.  Applies on top of my "Track idleness independent of idle tasks"
commit.  Right...  And the tracing relies on a later patch, so feel free
to yank the calls to trace_rcu_dyntick() on the off-chance that you are
crazy enough to actually try this.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

Not-yet-signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 06c0ed4..d4247e0 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -356,6 +356,11 @@ void rcu_idle_enter(void)
 
 	local_irq_save(flags);
 	rdtp = &__get_cpu_var(rcu_dynticks);
+	if (!idle_cpu(smp_processor_id())) {
+		trace_rcu_dyntick("--|", rdtp->dynticks_nesting);
+		local_irq_restore(flags);
+		return;
+	}
 	if (--rdtp->dynticks_nesting) {
 		trace_rcu_dyntick("--=", rdtp->dynticks_nesting);
 		local_irq_restore(flags);
@@ -384,6 +389,11 @@ void rcu_idle_exit(void)
 
 	local_irq_save(flags);
 	rdtp = &__get_cpu_var(rcu_dynticks);
+	if (!idle_cpu(smp_processor_id())) {
+		trace_rcu_dyntick("++|", rdtp->dynticks_nesting);
+		local_irq_restore(flags);
+		return;
+	}
 	if (rdtp->dynticks_nesting++) {
 		trace_rcu_dyntick("++=", rdtp->dynticks_nesting);
 		local_irq_restore(flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/