linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: nicolas.pitre@linaro.org, shreyas@linux.vnet.ibm.com,
	linux-kernel@vger.kernel.org, peterz@infradead.org,
	rafael@kernel.org, vincent.guittot@linaro.org
Subject: Re: [PATCH V4] irq: Track the interrupt timings
Date: Fri, 10 Jun 2016 16:52:25 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.11.1606101625320.5839@nanos> (raw)
In-Reply-To: <1460545556-15085-1-git-send-email-daniel.lezcano@linaro.org>

Thanks,

On Wed, 13 Apr 2016, Daniel Lezcano wrote:
> +static inline void setup_timings(struct irq_desc *desc, struct irqaction *act)
> +{
> +	/*
> +	 * Timers are deterministic, so no need to do any measurement

Deterministic is a confusing wording here. We don't need the measurement
because the idle code already knows the next expiry event.

> @@ -0,0 +1,133 @@
> +/*
> + * linux/kernel/irq/timings.c
> + *
> + * Copyright (C) 2016, Linaro Ltd - Daniel Lezcano <daniel.lezcano@linaro.org>
> + *

Please add an explicit License one liner. The folks doing license analysis
will be grateful.

> +DEFINE_STATIC_KEY_FALSE(irq_timing_enabled);
> +
> +void irq_timings_get(void)

_enable/disable() please. 

> +{
> +	static_branch_inc(&irq_timing_enabled);
> +}
> +
> +void irq_timings_put(void)
> +{
> +	static_branch_dec(&irq_timing_enabled);
> +}
> +
> +/**
> + * __handle_timings - stores an irq timing when an interrupt occurs
> + *
> + * @desc: the irq descriptor
> + *
> + * For all interruptions with their IRQS_TIMINGS flag set, the function
> + * computes the time interval between two interrupt events and store it
> + * in a circular buffer.
> + */
> +void __handle_timings(struct irq_desc *desc)
> +{
> +	struct irq_timings *timings;
> +	u64 prev, now, diff;
> +
> +	if (!(desc->istate & IRQS_TIMINGS))
> +		return;

Please have this check in the handle_timings() inline. No need to take a
function call for the timer interrupt.

> +
> +	timings = this_cpu_ptr(desc->timings);
> +	now = local_clock();
> +	prev = timings->timestamp;
> +	timings->timestamp = now;
> +
> +	/*
> +	 * If it is the first interrupt of the series, we can't
> +	 * compute an interval, just store the timestamp and exit.
> +	 */
> +	if (unlikely(!prev))
> +		return;

The delta will be large enough that you drop out in the check below. So you
can spare that conditional.

> +
> +	diff = now - prev;
> +
> +	/*
> +	 * microsec (actually 1024th of a milisec) precision is good
> +	 * enough for our purpose.
> +	 */
> +	diff >>= 10;

And that shift instruction is required because of the following?

>   	 * Otherwise we know the magnitude of diff is
> +	 * well within 32 bits.

AFAICT that's pointless. You are not saving anything because NSEC_PER_SEC is
smaller than 2^32 and your 8 values are not going to overflow 64 bit in the
sum.

> +	 */
> +	if (unlikely(diff > USEC_PER_SEC)) {
> +		memset(timings, 0, sizeof(*timings));
> +		timings->timestamp = now;

Redundant store.

> +		return;
> +	}
> +
> +	/* The oldest value corresponds to the next index. */
> +	timings->w_index = (timings->w_index + 1) & IRQ_TIMINGS_MASK;
> +
> +	/*
> +	 * Remove the oldest value from the summing. If this is the
> +	 * first time we go through this array slot, the previous
> +	 * value will be zero and we won't substract anything from the
> +	 * current sum. Hence this code relies on a zero-ed structure.
> +	 */
> +	timings->sum -= timings->values[timings->w_index];
> +	timings->values[timings->w_index] = diff;
> +	timings->sum += diff;

Now the real question is whether you really need all that math, checks and
memsets in the irq hotpath. If you make the storage slightly larger then you
can just store the values unconditionally in the circular buffer and do all
the computational stuff when you really it.

Thanks,

	tglx

  parent reply	other threads:[~2016-06-10 14:54 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-13 11:05 Daniel Lezcano
2016-04-22 18:21 ` Daniel Lezcano
2016-05-03 13:44 ` Daniel Lezcano
2016-06-10 14:52 ` Thomas Gleixner [this message]
2016-06-10 15:11   ` Nicolas Pitre
2016-06-10 15:20     ` Thomas Gleixner
2016-06-10 15:29       ` Nicolas Pitre
2016-06-14 13:36   ` Daniel Lezcano
2016-06-14 15:10     ` Nicolas Pitre
2016-06-14 15:38       ` Daniel Lezcano
2016-06-14 16:36       ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.11.1606101625320.5839@nanos \
    --to=tglx@linutronix.de \
    --cc=daniel.lezcano@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nicolas.pitre@linaro.org \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    --cc=shreyas@linux.vnet.ibm.com \
    --cc=vincent.guittot@linaro.org \
    --subject='Re: [PATCH V4] irq: Track the interrupt timings' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).