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.1606101128180.1714@knanqh.ubzr>
Date:	Fri, 10 Jun 2016 11:29:52 -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:

> On Fri, 10 Jun 2016, Nicolas Pitre wrote:
> > On Fri, 10 Jun 2016, Thomas Gleixner wrote:
> > > > +	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.
> 
> Well, you can do sum >> 10 exaclty once when you calculate stuff.

Given your later argument I agree.

> > > > +	 */
> > > > +	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.
> 
> So why doing a full memset and not only on the array ?

Go figure.  This code has come a long way.

> > > 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.
> 
> Glad you agree.
> 
> Thanks,
> 
> 	tglx
>