Hi Peter, On Wed, Oct 4, 2017 at 9:01 AM, Peter Zijlstra <pet...@infradead.org> wrote: > On Fri, Sep 29, 2017 at 02:22:45PM -0700, Joel Fernandes wrote: [...] >> + */ >> +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?
In lockdep code, when trace_hardirqs_off is called, trace_hardirqs_off_caller would pass CALLER_ADDR0 as trace_hardirqs_off. Because of this, the first argument passed to time_hardirqs_off would always be an offset within trace_hardirqs_off: time_hardirqs_off(CALLER_ADDR0, ip); Is that intended? Seems to me that in the lockdep implementation of trace_hardirqs_* in terms of *_caller(), we would completely miss the second-last return address (CALLER_ADDR1) of trace_hardirqs_off(). Also for the above reasons, I don't think it doesn't make sense to use this reuse logic for the tracer. Atleast I feel it might change the current behavior of the preempt/irqsoff tracer which I don't intend to change with my current patch set. thanks, - Joel