lkml.org 
[lkml]   [2015]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: call_rcu from trace_preempt
    On Mon, Jun 15, 2015 at 10:45:05PM -0700, Alexei Starovoitov wrote:
    > On 6/15/15 7:14 PM, Paul E. McKenney wrote:
    > >
    > >Why do you believe that it is better to fix it within call_rcu()?
    >
    > found it:
    > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
    > index 8cf7304b2867..a3be09d482ae 100644
    > --- a/kernel/rcu/tree.c
    > +++ b/kernel/rcu/tree.c
    > @@ -935,9 +935,9 @@ bool notrace rcu_is_watching(void)
    > {
    > bool ret;
    >
    > - preempt_disable();
    > + preempt_disable_notrace();
    > ret = __rcu_is_watching();
    > - preempt_enable();
    > + preempt_enable_notrace();
    > return ret;
    > }
    >
    > the rcu_is_watching() and __rcu_is_watching() are already marked
    > notrace, so imo it's a good 'fix'.
    > What was happening is that the above preempt_enable was triggering
    > recursive call_rcu that was indeed messing 'rdp' that was
    > prepared by __call_rcu and before __call_rcu_core could use that.

    > btw, also noticed that local_irq_save done by note_gp_changes
    > is partially redundant. In __call_rcu_core path the irqs are
    > already disabled.

    But you said earlier that nothing happened when interrupts were
    disabled. And interrupts are disabled across the call to
    rcu_is_watching() in __call_rcu_core(). So why did those calls
    to preempt_disable() and preempt_enable() cause trouble?

    That said, the patch looks inoffensive to me, adding Steven for his
    trace expertise.

    Still, I do need to understand what was really happening. Did interrupts
    get enabled somehow? Or is your code that ignores calls when interrupts
    are disabled incomplete in some way? Something else?

    > >Perhaps you are self-deadlocking within __call_rcu_core(). If you have
    > >not already done so, please try running with CONFIG_PROVE_LOCKING=y.
    >
    > yes, I had CONFIG_PROVE_LOCKING on.

    Good! ;-)

    > >I suspect that your problem may range quite a bit further than just
    > >call_rcu(). For example, in your stack trace, you have a recursive
    > >call to debug_object_activate(), which might not be such good thing.
    >
    > nope :) recursive debug_object_activate() is fine.
    > with the above 'fix' the trace.patch is now passing.
    >
    > Why I'm digging into all of these? Well, to find out when
    > it's safe to finally do call_rcu. If I will use deferred kfree
    > approach in bpf maps, I need to know when it's safe to finally
    > call_rcu and it's not an easy answer.

    Given that reentrant calls to call_rcu() and/or kfree_rcu() were not
    in any way considered during design and implementation, it is not a
    surprise that the answer is not easy. The reason I need to understand
    what your code does in interrupt-disabled situations is to work out
    whether or not it makes sense to agree to support reentrancy in call_rcu()
    and kfree_rcu().

    > kprobes potentially can be placed in any part of call_rcu stack,
    > so things can go messy quickly. So it helps to understand the call_rcu
    > logic well enough to come up with good solution.

    Indeed, I do have some concerns about that sort of thing, as it is not
    at all clear that designing call_rcu() and kfree_rcu() for unrestricted
    reentrancy is a win.

    Thanx, Paul



    \
     
     \ /
      Last update: 2015-06-16 14:41    [W:2.536 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site