From: Byungchul Park <byungchul.park@lge.com>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
jiangshanlai@gmail.com, dipankar@in.ibm.com,
mathieu.desnoyers@efficios.com, josh@joshtriplett.org,
rostedt@goodmis.org, luto@kernel.org, kernel-team@lge.com
Subject: Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts
Date: Fri, 15 Mar 2019 16:44:52 +0900 [thread overview]
Message-ID: <eea72fb5-c25d-746a-fa48-6d04c8d5d1af@lge.com> (raw)
In-Reply-To: <20190315073138.GB15148@X58A-UD3R>
On 03/15/2019 04:31 PM, Byungchul Park wrote:
> On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
>> On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
>>> RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
>>> either an interrupt that invokes rcu_irq_enter() but never invokes the
>>> corresponding rcu_irq_exit() on the one hand, or an interrupt that never
>>> invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
>>> on the other. These things really did happen at one time, as evidenced
>>> by this ca-2011 LKML post:
>>>
>>> http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
>>>
>>> The reason why RCU tolerates half-interrupts is that usermode helpers
>>> used exceptions to invoke a system call from within the kernel such that
>>> the system call did a normal return (not a return from exception) to
>>> the calling context. This caused rcu_irq_enter() to be invoked without
>>> a matching rcu_irq_exit(). However, usermode helpers have since been
>>> rewritten to make much more housebroken use of workqueues, kernel threads,
>>> and do_execve(), and therefore should no longer produce half-interrupts.
>>> No one knows of any other source of half-interrupts, but then again,
>>> no one seems insane enough to go audit the entire kernel to verify that
>>> half-interrupts really are a relic of the past.
>>>
>>> This commit therefore adds a pair of WARN_ON_ONCE() calls that will
>>> trigger in the presence of half interrupts, which the code will continue
>>> to handle correctly. If neither of these WARN_ON_ONCE() trigger by
>>> mid-2021, then perhaps RCU can stop handling half-interrupts, which
>>> would be a considerable simplification.
>> Hi Paul and everyone,
>> I was thinking some more about this patch and whether we can simplify this code
>> much in 2021. Since 2021 is a bit far away, I thought working on it in again to
>> keep it fresh in memory is a good idea ;-)
>>
>> To me it seems we cannot easily combine the counters (dynticks_nesting and
>> dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
>> half-interrupt scenario (assuming simplication means counter combining like
>> Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
>> 2 counters need to be tracked separately as they are used differently in the
>> following function:
> Hi Joel and Paul,
>
> I always love the way to logically approach problems so I'm a fan of
> all your works :) But I'm JUST curious about something here. Why can't
> we combine them the way I tried even if we confirm no possibility of
> half-interrupt? IMHO, the only thing we want to know through calling
> rcu_is_cpu_rrupt_from_idle() is whether the interrupt comes from
> RCU-idle or not - of course assuming the caller context always be an
> well-defined interrupt context like e.g. the tick handler.
>
> So the function can return true if the caller is within a RCU-idle
> region except a well-known single interrupt nested.
>
> Of course, now that we cannot confirm it yet, the crowbar is necessary.
> But does it still have a problem even after confirming it? Why? What am
> I missing? Could you explain why for me? :(
Did you also want to consider the case the function is called from
others than
well-known interrupt contexts? If yes, then I agree with you, there doesn't
seem to be the kind of code and it's not a good idea to let the function be
called generally though.
> Thanks,
> Byungchul
>
>> static int rcu_is_cpu_rrupt_from_idle(void)
>> {
>> return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
>> __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
>> }
>>
>> dynticks_nesting actually tracks if we entered/exited idle or user mode.
>>
>> dynticks_nmi_nesting tracks if we entered/exited interrupts.
>>
>> We have to do the "dynticks_nmi_nesting <= 1" check because
>> rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
>> (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
>> check is because the CPU MUST be in user or idle for the check to return
>> true. We can't really combine these two into one counter then I think because
>> they both convey different messages.
>>
>> The only simplication we can do, is probably the "crowbar" updates to
>> dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
>> no more half-interrupts are possible. Which might still be a worthwhile thing
>> to do (while still keeping both counters separate).
>>
>> However, I think we could combine the counters and lead to simplying the code
>> in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
>> not need the counters but NOHZ_FULL may take issue with that since it needs
>> rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.
>>
>> Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
>> In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
>> will really get fired for half-interrupts. The vast majority of the systems I believe are
>> NOHZ_IDLE not NOHZ_FULL.
>> This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
>> rcu_irq_enter() [half interrupt causes an exception and thus rcu_irq_enter]
>> rcu_user_enter() [due to usermode upcall]
>> rcu_user_exit()
>> (no more rcu_irq_exit() - hence half an interrupt)
>>
>> But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
>> rcu_eqs_e{xit,nter} really do anything?
>>
>> Or was the idea with adding the new warnings, that they would fire the next
>> time rcu_idle_enter/exit is called? Like for example:
>>
>> rcu_irq_enter() [This is due to half-interrupt]
>> rcu_idle_enter() [Eventually we enter the idle loop at some point
>> after the half-interrupt and the rcu_eqs_enter()
>> would "crowbar" the dynticks_nmi_nesting counter to 0].
>>
>> thanks!
>>
>> - Joel
>>
>>> Reported-by: Steven Rostedt <rostedt@goodmis.org>
>>> Reported-by: Joel Fernandes <joel@joelfernandes.org>
>>> Reported-by: Andy Lutomirski <luto@kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>>> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> ---
>>> kernel/rcu/tree.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>>> index dc041c2afbcc..d2b6ade692c9 100644
>>> --- a/kernel/rcu/tree.c
>>> +++ b/kernel/rcu/tree.c
>>> @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
>>> struct rcu_dynticks *rdtp;
>>>
>>> rdtp = this_cpu_ptr(&rcu_dynticks);
>>> + WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
>>> WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
>>> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>>> rdtp->dynticks_nesting == 0);
>>> @@ -895,6 +896,7 @@ static void rcu_eqs_exit(bool user)
>>> trace_rcu_dyntick(TPS("End"), rdtp->dynticks_nesting, 1, rdtp->dynticks);
>>> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
>>> WRITE_ONCE(rdtp->dynticks_nesting, 1);
>>> + WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
>>> WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
>>> }
>>>
>>> --
>>> 2.17.1
>>>
next prev parent reply other threads:[~2019-03-15 7:45 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-29 22:20 [PATCH tip/core/rcu 0/19] RCU flavor-consolidation changes for v4.20/v5.0 Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 01/19] rcu: Refactor rcu_{nmi,irq}_{enter,exit}() Paul E. McKenney
2018-08-30 18:10 ` Steven Rostedt
2018-08-30 23:02 ` Paul E. McKenney
2018-08-31 2:25 ` Byungchul Park
2018-08-29 22:20 ` [PATCH tip/core/rcu 02/19] rcu: Defer reporting RCU-preempt quiescent states when disabled Paul E. McKenney
2018-10-29 11:24 ` Ran Rozenstein
2018-10-29 14:27 ` Paul E. McKenney
2018-10-30 3:44 ` Joel Fernandes
2018-10-30 12:58 ` Paul E. McKenney
2018-10-30 22:21 ` Joel Fernandes
2018-10-31 18:22 ` Paul E. McKenney
2018-11-02 19:43 ` Paul E. McKenney
2018-11-26 13:55 ` Ran Rozenstein
2018-11-26 19:00 ` Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 03/19] rcutorture: Test extended "rcu" read-side critical sections Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 04/19] rcu: Allow processing deferred QSes for exiting RCU-preempt readers Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 05/19] rcu: Remove now-unused ->b.exp_need_qs field from the rcu_special union Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts Paul E. McKenney
2019-03-11 13:39 ` Joel Fernandes
2019-03-11 22:29 ` Paul E. McKenney
2019-03-12 15:05 ` Joel Fernandes
2019-03-12 15:20 ` Paul E. McKenney
2019-03-13 15:09 ` Joel Fernandes
2019-03-13 15:27 ` Steven Rostedt
2019-03-13 15:51 ` Paul E. McKenney
2019-03-13 16:51 ` Steven Rostedt
2019-03-13 18:07 ` Paul E. McKenney
2019-03-14 12:31 ` Joel Fernandes
2019-03-14 13:36 ` Steven Rostedt
2019-03-14 13:37 ` Steven Rostedt
2019-03-14 21:27 ` Joel Fernandes
2019-03-15 7:31 ` Byungchul Park
2019-03-15 7:44 ` Byungchul Park [this message]
2019-03-15 13:46 ` Joel Fernandes
2018-08-29 22:20 ` [PATCH tip/core/rcu 07/19] rcu: Apply RCU-bh QSes to RCU-sched and RCU-preempt when safe Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 08/19] rcu: Report expedited grace periods at context-switch time Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 09/19] rcu: Define RCU-bh update API in terms of RCU Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 10/19] rcu: Update comments and help text for no more RCU-bh updaters Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 11/19] rcu: Drop "wake" parameter from rcu_report_exp_rdp() Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 12/19] rcu: Fix typo in rcu_get_gp_kthreads_prio() header comment Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 13/19] rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 14/19] rcu: Express Tiny RCU updates in terms of RCU rather than RCU-sched Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 15/19] rcu: Remove RCU_STATE_INITIALIZER() Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 16/19] rcu: Eliminate rcu_state structure's ->call field Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 17/19] rcu: Remove rcu_state structure's ->rda field Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 18/19] rcu: Remove rcu_state_p pointer to default rcu_state structure Paul E. McKenney
2018-08-29 22:20 ` [PATCH tip/core/rcu 19/19] rcu: Remove rcu_data_p pointer to default rcu_data structure Paul E. McKenney
2018-08-29 22:22 ` [PATCH tip/core/rcu 0/19] RCU flavor-consolidation changes for v4.20/v5.0 Paul E. McKenney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eea72fb5-c25d-746a-fa48-6d04c8d5d1af@lge.com \
--to=byungchul.park@lge.com \
--cc=dipankar@in.ibm.com \
--cc=jiangshanlai@gmail.com \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=kernel-team@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).