Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934601AbdCWTCc (ORCPT ); Thu, 23 Mar 2017 15:02:32 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34636 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752106AbdCWTCa (ORCPT ); Thu, 23 Mar 2017 15:02:30 -0400 Date: Thu, 23 Mar 2017 20:02:25 +0100 From: Daniel Lezcano To: Peter Zijlstra Cc: tglx@linutronix.de, linux-kernel@vger.kernel.org, nicolas.pitre@linaro.org, rafael@kernel.org, vincent.guittot@linaro.org Subject: Re: [PATCH V8 2/3] irq: Track the interrupt timings Message-ID: <20170323190225.GB24630@mai> References: <1490290924-12958-1-git-send-email-daniel.lezcano@linaro.org> <1490290924-12958-2-git-send-email-daniel.lezcano@linaro.org> <20170323183542.xfhc4zhjxbyxar57@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170323183542.xfhc4zhjxbyxar57@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2147 Lines: 73 On Thu, Mar 23, 2017 at 07:35:42PM +0100, Peter Zijlstra wrote: > On Thu, Mar 23, 2017 at 06:42:02PM +0100, Daniel Lezcano wrote: > > +/* > > + * The function record_irq_time is only called in one place in the > > + * interrupts handler. We want this function always inline so the code > > + * inside is embedded in the function and the static key branching > > + * code can act at the higher level. Without the explicit > > + * __always_inline we can end up with a function call and a small > > + * overhead in the hotpath for nothing. > > + */ > > +static __always_inline void record_irq_time(struct irq_desc *desc) > > +{ > > + if (static_key_enabled(&irq_timing_enabled)) { > > I think you meant to have either static_branch_likely() or > static_branch_unlikely() here. Those are runtime code patched, > static_key_enabled() generates a regular load and test condition. > > Also; if you do something like: > > if (!static_branch_likely(&irq_timing_enabled)) > return; > > you can save one level of indent. Ok, thanks for the hint. > > + if (desc->istate & IRQS_TIMINGS) { > > + struct irq_timings *timings = this_cpu_ptr(&irq_timings); > > + unsigned int index = timings->count & IRQ_TIMINGS_MASK; > > + > > + timings->values[index].ts = local_clock(); > > + timings->values[index].irq = irq_desc_get_irq(desc); > > + timings->count++; > > + } > > + } > > +} > > > > > +DEFINE_STATIC_KEY_FALSE(irq_timing_enabled); > > + > > +DEFINE_PER_CPU(struct irq_timings, irq_timings); > > + > > +void irq_timings_enable(void) > > +{ > > + static_branch_inc(&irq_timing_enabled); > > Do you really need counting, or do you want static_branch_enable() here? I put counting in order to let several subsystem to use the irq timings if it is needed. > > +} > > + > > +void irq_timings_disable(void) > > +{ > > + static_branch_dec(&irq_timing_enabled); > > idem. > > > +} > > -- > > 1.9.1 > > -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog