Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 Jan 2021 14:42:32 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     Neeraj Upadhyay <neeraju@codeaurora.org>, josh@joshtriplett.org,
        rostedt@goodmis.org, mathieu.desnoyers@efficios.com,
        jiangshanlai@gmail.com, joel@joelfernandes.org,
        rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in
 rcu_is_cpu_rrupt_from_idle
Message-ID: <20210105134232.GI3040@hirez.programming.kicks-ass.net>
References: <1608712777-1769-1-git-send-email-neeraju@codeaurora.org>
 <20201223151231.GC2657@paulmck-ThinkPad-P72>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201223151231.GC2657@paulmck-ThinkPad-P72>
Precedence: bulk

On Wed, Dec 23, 2020 at 07:12:31AM -0800, Paul E. McKenney wrote:
> On Wed, Dec 23, 2020 at 02:09:37PM +0530, Neeraj Upadhyay wrote:
> > For the smp_call_function() optimization, where callbacks can run from
> > idle context, in commit 806f04e9fd2c ("rcu: Allow for smp_call_function()
> > running callbacks from idle"), an additional check is added in
> > rcu_is_cpu_rrupt_from_idle(), for dynticks_nmi_nesting value being 0,
> > for these smp_call_function() callbacks running from idle loop.
> > However, this commit missed updating a preexisting underflow check
> > of dynticks_nmi_nesting, which checks for a non zero positive value.
> > Fix this warning and while at it, read the counter only once.
> > 
> > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> > ---
> > 
> > Hi,
> > 
> > I was not able to get this warning, with scftorture.
> > 
> >   RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> >     "RCU dynticks_nmi_nesting counter underflow/zero!");
> > 
> > Not sure if idle loop smp_call_function() optimization is already present
> > in mainline?
> 
> Now that you mention it, I don't see it.

kernel/sched/idle.c:do_idle() calls flush_smp_call_function_from_idle().

(nothing x86 specific about it)

> > Another thing, which I am not sure of is, maybe lockdep gets disabled
> > in the idle loop contexts, where rcu_is_cpu_rrupt_from_idle() is called?
> > Was this the original intention, to keep the lockdep based
> > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0
> > check separate from idle task context nesting value
> > WARN_ON_ONCE(!nesting && !is_idle_task(current)) check?
> 
> An easy way to test lockdep is to create a pair of locks, acquire them
> in one order then release them both, and finally acquire them in the
> other order and then release them both.  If lockdep is configured and
> enabled, it will complain.

IIRC (and this is after not staring at the computer for 2 weeks) lockdep
should work just fine in idle, except of course that RCU will be stopped
so actually taking locks will scream bloody murder due to tracing etc..

> The only reason I used RCU_LOCKDEP_WARN() was that people were complaining
> to me about idle-entry overhead back at that time.  So without lockdep,
> there is zero overhead.  Maybe people have become more tolerant of idle
> delays, or perhaps they are not so worried about an extra check of a
> cache-hot quantity.

Not having checks also saves on $I and branches, in general I think
having checks depend on DEBUG features, esp. those we don't really
expect to trigger is still sane.

> I am tempted to pull this in as is, given the current logical
> inconsistency in the checks.  Thoughts?

Patch looks ok, although I've seen compilers do CSE on
__this_cpu_read() (on x86).