Messages in this thread | | | Date | Fri, 10 Jun 2016 17:20:43 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH V4] irq: Track the interrupt timings |
| |
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.
> > > + */ > > > + 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 ?
> > 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
| |