Date: Thu, 23 Mar 2017 19:35:42 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
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: <20170323183542.xfhc4zhjxbyxar57@hirez.programming.kicks-ass.net>
References: <1490290924-12958-1-git-send-email-daniel.lezcano@linaro.org>
 <1490290924-12958-2-git-send-email-daniel.lezcano@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1490290924-12958-2-git-send-email-daniel.lezcano@linaro.org>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1618
Lines: 59

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.

> +		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?

> +}
> +
> +void irq_timings_disable(void)
> +{
> +	static_branch_dec(&irq_timing_enabled);

idem.

> +}
> -- 
> 1.9.1
>