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
>
