From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756019Ab1GLWJW (ORCPT ); Tue, 12 Jul 2011 18:09:22 -0400 Received: from smtp-out.google.com ([216.239.44.51]:27280 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755119Ab1GLWJV convert rfc822-to-8bit (ORCPT ); Tue, 12 Jul 2011 18:09:21 -0400 DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=dkim-signature:mime-version:in-reply-to:references:from:date: message-id:subject:to:cc:content-type: content-transfer-encoding:x-system-of-record; b=njnrufo/8KFkUyyFUZFB4qjCrUdqcEETvqQ5gHv6ycwkZNhIZLG4zo9kN0DoehbLA s/nard7n3v6eIqxPb8Ctw== MIME-Version: 1.0 In-Reply-To: <20110712180859.GC9201@somewhere> References: <1306877298-31713-1-git-send-email-vnagarnaik@google.com> <1308681903-12840-1-git-send-email-vnagarnaik@google.com> <20110707233453.GE21115@somewhere> <20110711155404.GB4109@somewhere.redhat.com> <20110712180859.GC9201@somewhere> From: Vaibhav Nagarnaik Date: Tue, 12 Jul 2011 15:08:48 -0700 Message-ID: Subject: Re: [PATCH v3] trace: Add x86 irq vector entry/exit tracepoints To: Frederic Weisbecker Cc: David Sharp , Thomas Gleixner , Ingo Molnar , Steven Rostedt , Michael Rubin , linux-kernel@vger.kernel.org, x86@kernel.org, Jiaying Zhang Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 12, 2011 at 11:09 AM, Frederic Weisbecker 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 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