Messages in this thread | | | From | Joel Fernandes <> | Date | Wed, 4 Oct 2017 09:43:38 -0700 | Subject | Re: [PATCH v7 2/2] tracing: Add support for preempt and irq enable/disable events |
| |
Hi Peter,
On Wed, Oct 4, 2017 at 9:01 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Sep 29, 2017 at 02:22:45PM -0700, Joel Fernandes wrote: >> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c >> index 0e3033c00474..515ac851841a 100644 >> --- a/kernel/trace/trace_irqsoff.c >> +++ b/kernel/trace/trace_irqsoff.c >> @@ -16,6 +16,9 @@ >> >> #include "trace.h" >> >> +#define CREATE_TRACE_POINTS >> +#include <trace/events/preemptirq.h> >> + >> #if defined(CONFIG_IRQSOFF_TRACER) || defined(CONFIG_PREEMPT_TRACER) >> static struct trace_array *irqsoff_trace __read_mostly; >> static int tracer_enabled __read_mostly; >> @@ -776,27 +779,60 @@ static inline void tracer_preempt_on(unsigned long a0, unsigned long a1) { } >> static inline void tracer_preempt_off(unsigned long a0, unsigned long a1) { } >> #endif >> >> +/* >> + * trace_hardirqs_off can be called even when IRQs are already off. > > In fact it must be.. otherwise you'll get a complaint. > >> It is >> + * pointless and inconsistent with trace_preempt_enable and >> + * trace_preempt_disable to trace this, lets prevent double counting it with a >> + * per-cpu variable. Also reuse the per-cpu variable for other trace_hardirqs_* >> + * functions since we already define it. > > Lockdep ignores redundant calls. But I'm not entirely sure what the > above is trying to say.
I meant to say the same thing (that I'm trying to ignore redundant calls). In the last review you mentioned that it needs explaining, so I added a comment. Should I just remove the comment then? Or I could just state in a single line that the per-CPU variable is used for ignoring redundant calls.
>> + */ >> +static DEFINE_PER_CPU(int, tracing_irq_cpu); >> + >> #if defined(CONFIG_TRACE_IRQFLAGS) && !defined(CONFIG_PROVE_LOCKING) >> void trace_hardirqs_on(void) >> { >> + if (!this_cpu_read(tracing_irq_cpu)) >> + return; >> + >> + trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); >> tracer_hardirqs_on(); >> + >> + this_cpu_write(tracing_irq_cpu, 0); >> } >> EXPORT_SYMBOL(trace_hardirqs_on); >> >> void trace_hardirqs_off(void) >> { >> + if (this_cpu_read(tracing_irq_cpu)) >> + return; >> + >> + this_cpu_write(tracing_irq_cpu, 1); >> + >> + trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); >> tracer_hardirqs_off(); >> } >> EXPORT_SYMBOL(trace_hardirqs_off); >> >> __visible void trace_hardirqs_on_caller(unsigned long caller_addr) >> { >> + if (!this_cpu_read(tracing_irq_cpu)) >> + return; >> + >> + trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr); >> tracer_hardirqs_on_caller(caller_addr); >> + >> + this_cpu_write(tracing_irq_cpu, 0); >> } >> EXPORT_SYMBOL(trace_hardirqs_on_caller); >> >> __visible void trace_hardirqs_off_caller(unsigned long caller_addr) >> { >> + if (this_cpu_read(tracing_irq_cpu)) >> + return; >> + >> + this_cpu_write(tracing_irq_cpu, 1); >> + >> + trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr); >> tracer_hardirqs_off_caller(caller_addr); >> } >> EXPORT_SYMBOL(trace_hardirqs_off_caller); > > lockdep implements the trace_hardirq_*() in terms of *_caller(). Would > that make sense here?
Yes, I think that makes sense and reduces code duplication, I can do this in the next rev.
thanks,
- Joel
| |