Re: [PATCH v3] trace: Add x86 irq vector entry/exit tracepoints

[Vaibhav Nagarnaik]

On Tue, Jul 12, 2011 at 11:09 AM, Frederic Weisbecker
<fweisbec@xxxxxxxxx> wrote:
> On Mon, Jul 11, 2011 at 11:21:04AM -0700, Vaibhav Nagarnaik wrote:
>> On Mon, Jul 11, 2011 at 8:54 AM, Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:
>> > Let me clarify what I proposed, we have that:
>> >
>> >        irq_enter() // trace_irq_enter();
>> >
>> >        trace_irq_vector_entry(foo);
>> >
>> >        // handle irq...
>> >
>> >        irq_exit() // trace_irq_exit()
>> >
>> > So either you're only interested in tracing raw irqs and you don't care
>> > about the nature of these and you only enable trace_irq_enter() and
>> > trace_irq_exit(). Or you're interested in the nature of these and you
>> > only enable trace_irq_vector_entry() and trace_irq_exit().
>> >
>> > In fact we could even drop the trace_irq_enter() thing because I'm
>> > not sure it's interesting.
>> >
>> > All in one it doesn't bring more overhead.
>> >
>> > What I'm more worried about actually is that in the case of trace_irq_handler_entry()
>> > and trace_irq_handler_exit(), it becomes a useless tracepoint. And if you enable it
>> > for other irq tracepoint symetric purposes, you'll have it there too and that's a bit
>> > odd.
>> >
>> > So perhaps we want to keep your way of doing this.
>> >
>>
>> We proposed this patch as a way of tracing raw interrupts from the
>> platform, which is why it traces all the raw interrupts and does not use
>> any specific interrupt data in the events. So when someone wants to know
>> just the interrupts that are being triggered in the system, this
>> tracepoint can be used.
>
> Which one? Not sure I understand what you mean.
>
>> There can be more involved and detailed tracepoints for individual IRQs,
>> as you proposed to have trace_ipi_function(), etc.
>
> I was arguing about having a tracepoint in irq_exit(), so I guess we are not talking about the same
> things.
>
>> > On Thu, Jul 07, 2011 at 05:54:02PM -0700, David Sharp wrote:
>> >> Here's our motivation: We use interrupt events, along with traps,
>> >> syscall, and sched_switch events to know when kernel code is running
>> >> instead of user code. So, being clear that we got all interrupts (and
>> >> which interrupt) is important.
>> >
>> > Ok I see your point then.
>> >
>> > But then have a generic single trace_irq_timer() (may be cut down in entry/exit if
>> > you want).
>> >
>> > But I don't think we really want to  into HRTIMER, PERIODIC, ONESHOT, BROADCAST
>> > vectors. That's sounds like the wrong place to provide these details.
>> >
>> > If we want higher level details, then we can enable hrtimer tracepoints, and/or we add some
>> > tracepoints in the timer subsystem to know with what we are dealing with.
>> >
>> > Does that make sense?
>> >
>>
>> I added the details about the timer handler because it would be
>> difficult to understand exactly which generic event handlers the
>> platform LOCAL_TIMER_IRQ_VECTOR led to.
>
> Generic event handler is already a high level detail. At the point we call
> that generic event handler, we have climbed several steps, we have left the
> world of pure irq vectors.
>
> What is normally suitable here to get more semantic details is to use higher level
> tracepoints like high resolution timer ones.
>
> If these are not enough for you, you can add a trace_hrtimer_interrupt()
> in the irq tracepoint subsystem. If it's in the irq subsystem you include
> it with the rest of irq traces.
>
>> Once again, this is meant as a raw interrupt tracepoint which provides 2
>> things: an interrupt occurred and the nature of interrupt. As you
>> mentioned there are high level hrtimer tracepoints and there can be
>> similar highly detailed tracepoints for each IRQ as required with more
>> data about the interrupt, but I believe this is the wrong patch to put
>> them in.
>
> But the problem is you are actually messing up irq raw events and higher
> level details.
>
> That vector space should lay in the arch low ground and never go further.
> Finding one of these trace vector things elsewhere than arch/ shouldn't
> ever happen.
>
> As far as possible, tracepoints must be self-contained events and be
> self-explanatory. We want granularity there.
>
> There should never be a reschedule vector to trace. That event is self
> contained and wants its own tracepoints. irq/reschedule_interrupt is
> something any kernel developer can understand and give some sense out
> by browsing the events. But irq/interrupt_vector is opaque as hell.
>
> What happens if I want to trace only reschedule interrupts? I'd be
> forced to enable that vector, browse its format, find the value
> for the reschedule vector, apply a filter.
> That requires a lot of time and skills compared to just enable a tracepoint.
>
> Your patch mixes up too much things:
>
> - pure arch tracepoints. Only those want that vector naming I think.
>
> - IPIs that should go to kernel/smp.c
>
> - reschedule interrupt
>
> - irq work
>
> - ...
>
> If you cut the things into more self-contained topics, this is going to be
> much easier to review and thus much faster to apply and push.
>
> Also your goal is to not miss any interrupts. So if everything goes to the irq
> tracepoint subsystem, you just need to enable everything there and you are
> done. You don't need a big vector tracepoint to ensure you won't miss something.
>

Breaking this patch up in different small ones makes sense. Can you
comment on this proposal for the following trace events?

For tracepoints in generic IRQ handlers:
1. trace_timer_vector - takes an enum for BROADCAST, HRTIMER, ONESHOT,
   PERIODIC and NOHZ.
2. trace_irq_work_vector - for IRQ_WORK_VECTOR
3. trace_reschedule_vector - for RESCHEDULE_IPI vector
4. trace_call_function_vector - takes an enum for CALL_FUNCTION and
   CALL_FUNCTION_SINGLE

Another trace event for arch-specific IRQ vectors which don't have
generic event handlers:
5. trace_platform_irq_vector - takes an enum, which is defined in
   asm/irq.h for each platform. This is traced in arch-specific files
   only.

How does this sound?



Vaibhav Nagarnaik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/