lkml.org 
[lkml]   [2019]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts
    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? :(

    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
    > >

    \
     
     \ /
      Last update: 2019-03-15 09:02    [W:2.584 / U:0.428 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site