lkml.org 
[lkml]   [2018]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 6/8] tracing: Centralize preemptirq tracepoints and unify their usage
    Hi Joel,

    On Wed, May 23, 2018 at 06:21:55PM -0700, Joel Fernandes wrote:
    > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
    >
    > This patch detaches the preemptirq tracepoints from the tracers and
    > keeps it separate.
    >
    > Advantages:
    > * Lockdep and irqsoff event can now run in parallel since they no longer
    > have their own calls.
    >
    > * This unifies the usecase of adding hooks to an irqsoff and irqson
    > event, and a preemptoff and preempton event.
    > 3 users of the events exist:
    > - Lockdep
    > - irqsoff and preemptoff tracers
    > - irqs and preempt trace events
    >
    > The unification cleans up several ifdefs and makes the code in preempt
    > tracer and irqsoff tracers simpler. It gets rid of all the horrific
    > ifdeferry around PROVE_LOCKING and makes configuration of the different
    > users of the tracepoints more easy and understandable. It also gets rid
    > of the time_* function calls from the lockdep hooks used to call into
    > the preemptirq tracer which is not needed anymore. The negative delta in
    > lines of code in this patch is quite large too.
    >
    > In the patch we introduce a new CONFIG option PREEMPTIRQ_TRACEPOINTS
    > as a single point for registering probes onto the tracepoints. With
    > this,
    > the web of config options for preempt/irq toggle tracepoints and its
    > users becomes:
    >
    > PREEMPT_TRACER PREEMPTIRQ_EVENTS IRQSOFF_TRACER PROVE_LOCKING
    > | | \ | |
    > \ (selects) / \ \ (selects) /
    > TRACE_PREEMPT_TOGGLE ----> TRACE_IRQFLAGS
    > \ /
    > \ (depends on) /
    > PREEMPTIRQ_TRACEPOINTS
    >
    > One note, I have to check for lockdep recursion in the code that calls
    > the trace events API and bail out if we're in lockdep recursion
    > protection to prevent something like the following case: a spin_lock is
    > taken. Then lockdep_acquired is called. That does a raw_local_irq_save
    > and then sets lockdep_recursion, and then calls __lockdep_acquired. In
    > this function, a call to get_lock_stats happens which calls
    > preempt_disable, which calls trace IRQS off somewhere which enters my
    > tracepoint code and sets the tracing_irq_cpu flag to prevent recursion.
    > This flag is then never cleared causing lockdep paths to never be
    > entered and thus causing splats and other bad things.
    >
    > Other than the performance tests mentioned in the previous patch, I also
    > ran the locking API test suite. I verified that all tests cases are
    > passing.
    >
    > I also injected issues by not registering lockdep probes onto the
    > tracepoints and I see failures to confirm that the probes are indeed
    > working.
    >
    > Without probes:
    >
    > [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
    > [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
    > [ 0.000000] sirq-safe-A => hirqs-on/12: ok | ok | ok |
    > [ 0.000000] sirq-safe-A => hirqs-on/21: ok | ok | ok |
    > [ 0.000000] hard-safe-A + irqs-on/12: ok | ok | ok |
    > [ 0.000000] soft-safe-A + irqs-on/12: ok | ok | ok |
    > [ 0.000000] hard-safe-A + irqs-on/21: ok | ok | ok |
    > [ 0.000000] soft-safe-A + irqs-on/21: ok | ok | ok |
    > [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
    > [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
    >
    > With probes:
    >
    > [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
    > [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
    > [ 0.000000] sirq-safe-A => hirqs-on/12:FAILED|FAILED| ok |
    > [ 0.000000] sirq-safe-A => hirqs-on/21:FAILED|FAILED| ok |
    > [ 0.000000] hard-safe-A + irqs-on/12:FAILED|FAILED| ok |
    > [ 0.000000] soft-safe-A + irqs-on/12:FAILED|FAILED| ok |
    > [ 0.000000] hard-safe-A + irqs-on/21:FAILED|FAILED| ok |
    > [ 0.000000] soft-safe-A + irqs-on/21:FAILED|FAILED| ok |
    > [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
    > [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
    >
    > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    > ---

    [SNIP]
    >
    > #ifdef CONFIG_IRQSOFF_TRACER
    > +/*
    > + * We are only interested in hardirq on/off events:
    > + */
    > +static void tracer_hardirqs_on(void *none, unsigned long a0, unsigned long a1)
    > +{
    > + if (!preempt_trace() && irq_trace())
    > + stop_critical_timing(a0, a1);
    > +}
    > +
    > +static void tracer_hardirqs_off(void *none, unsigned long a0, unsigned long a1)
    > +{
    > + if (!preempt_trace() && irq_trace())
    > + start_critical_timing(a0, a1);
    > +}
    > +
    > static int irqsoff_tracer_init(struct trace_array *tr)
    > {
    > trace_type = TRACER_IRQS_OFF;
    >
    > + register_trace_irq_disable(tracer_hardirqs_off, NULL);
    > + register_trace_irq_enable(tracer_hardirqs_on, NULL);
    > return __irqsoff_tracer_init(tr);
    > }
    >
    > static void irqsoff_tracer_reset(struct trace_array *tr)
    > {
    > + unregister_trace_irq_disable(tracer_hardirqs_off, NULL);
    > + unregister_trace_irq_enable(tracer_hardirqs_on, NULL);
    > __irqsoff_tracer_reset(tr);
    > }
    >
    > @@ -692,19 +650,37 @@ static struct tracer irqsoff_tracer __read_mostly =
    > };
    > # define register_irqsoff(trace) register_tracer(&trace)
    > #else
    > +static inline void tracer_hardirqs_on(unsigned long a0, unsigned long a1) { }
    > +static inline void tracer_hardirqs_off(unsigned long a0, unsigned long a1) { }

    Just a nitpick. These lines seem unnecessary since they're used
    only when CONFIG_IRQSOFF_TRACER is enabled AFAICS.


    > # define register_irqsoff(trace) do { } while (0)
    > -#endif
    > +#endif /* CONFIG_IRQSOFF_TRACER */
    >
    > #ifdef CONFIG_PREEMPT_TRACER
    > +static void tracer_preempt_on(void *none, unsigned long a0, unsigned long a1)
    > +{
    > + if (preempt_trace() && !irq_trace())
    > + stop_critical_timing(a0, a1);
    > +}
    > +
    > +static void tracer_preempt_off(void *none, unsigned long a0, unsigned long a1)
    > +{
    > + if (preempt_trace() && !irq_trace())
    > + start_critical_timing(a0, a1);
    > +}
    > +
    > static int preemptoff_tracer_init(struct trace_array *tr)
    > {
    > trace_type = TRACER_PREEMPT_OFF;
    >
    > + register_trace_preempt_disable(tracer_preempt_off, NULL);
    > + register_trace_preempt_enable(tracer_preempt_on, NULL);
    > return __irqsoff_tracer_init(tr);
    > }
    >
    > static void preemptoff_tracer_reset(struct trace_array *tr)
    > {
    > + unregister_trace_preempt_disable(tracer_preempt_off, NULL);
    > + unregister_trace_preempt_enable(tracer_preempt_on, NULL);
    > __irqsoff_tracer_reset(tr);
    > }
    >
    > @@ -729,21 +705,32 @@ static struct tracer preemptoff_tracer __read_mostly =
    > };
    > # define register_preemptoff(trace) register_tracer(&trace)
    > #else
    > +static inline void tracer_preempt_on(unsigned long a0, unsigned long a1) { }
    > +static inline void tracer_preempt_off(unsigned long a0, unsigned long a1) { }

    Ditto (for CONFIG_PREEMPT_TRACER).

    Thanks,
    Namhyung


    > # define register_preemptoff(trace) do { } while (0)
    > -#endif
    > +#endif /* CONFIG_PREEMPT_TRACER */
    >
    > -#if defined(CONFIG_IRQSOFF_TRACER) && \
    > - defined(CONFIG_PREEMPT_TRACER)
    > +#if defined(CONFIG_IRQSOFF_TRACER) && defined(CONFIG_PREEMPT_TRACER)
    >
    > static int preemptirqsoff_tracer_init(struct trace_array *tr)
    > {
    > trace_type = TRACER_IRQS_OFF | TRACER_PREEMPT_OFF;
    >
    > + register_trace_irq_disable(tracer_hardirqs_off, NULL);
    > + register_trace_irq_enable(tracer_hardirqs_on, NULL);
    > + register_trace_preempt_disable(tracer_preempt_off, NULL);
    > + register_trace_preempt_enable(tracer_preempt_on, NULL);
    > +
    > return __irqsoff_tracer_init(tr);
    > }
    >
    > static void preemptirqsoff_tracer_reset(struct trace_array *tr)
    > {
    > + unregister_trace_irq_disable(tracer_hardirqs_off, NULL);
    > + unregister_trace_irq_enable(tracer_hardirqs_on, NULL);
    > + unregister_trace_preempt_disable(tracer_preempt_off, NULL);
    > + unregister_trace_preempt_enable(tracer_preempt_on, NULL);
    > +
    > __irqsoff_tracer_reset(tr);
    > }

    \
     
     \ /
      Last update: 2018-05-25 13:45    [W:3.514 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site