linux-kernel - Re: [PATCH V4] irq: Track the interrupt timings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.20.1606101059350.1714@knanqh.ubzr>
Date:	Fri, 10 Jun 2016 11:11:28 -0400 (EDT)
From:	Nicolas Pitre <nicolas.pitre@...aro.org>
To:	Thomas Gleixner <tglx@...utronix.de>
cc:	Daniel Lezcano <daniel.lezcano@...aro.org>,
	shreyas@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
	peterz@...radead.org, rafael@...nel.org, vincent.guittot@...aro.org
Subject: Re: [PATCH V4] irq: Track the interrupt timings

On Fri, 10 Jun 2016, Thomas Gleixner wrote:

> Thanks,
> 
> On Wed, 13 Apr 2016, Daniel Lezcano wrote:
> > +	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.

Fair enough.

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

Those values are squared later, so we really want 32 bits here.

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

We just trashed all our data with the memset so the current timestamp 
needs to be restored.

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

Well... given that you need an IRQ everytime you come out of idle that 
means there will always be more IRQs than entries into idle, so you're 
probably right.


Nicolas