Messages in this thread | | | Date | Tue, 7 Jul 2009 09:40:18 +0200 | From | Martin Schwidefsky <> | Subject | Re: [PATCH] optimized ktime_get[_ts] for GENERIC_TIME=y |
| |
On Mon, 6 Jul 2009 22:31:39 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 6 Jul 2009, Martin Schwidefsky wrote: > > +ktime_t ktime_get(void) > > +{ > > + cycle_t cycle_now, cycle_delta; > > + struct timespec time; > > + unsigned long seq; > > + s64 nsecs; > > + > > + do { > > + seq = read_seqbegin(&xtime_lock); > > + time.tv_sec = xtime.tv_sec + wall_to_monotonic.tv_sec; > > + time.tv_nsec = xtime.tv_nsec + wall_to_monotonic.tv_nsec; > > That's actually a violation of the timespec semantics. tv_nsec can end > up greater than (10^9 - 1). Please use separate sec and nsec variables.
Well the tv_sec/tv_nsec fields of a timespec are long values. But its no problem to switch to local variables.
> secs = xtime.tv_sec + wall_to_monotonic.tv_sec; > nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec; > > > + /* read clocksource: */ > > + cycle_now = clocksource_read(clock); > > + > > + /* calculate the delta since the last update_wall_time: */ > > + cycle_delta = (cycle_now - clock->cycle_last) & clock->mask; > > + > > + /* convert to nanoseconds: */ > > + nsecs = cyc2ns(clock, cycle_delta); > > So that needs to be changed to: > > nsecs += cyc2ns(clock, cycle_delta);
Ok.
> > + > > + } while (read_seqretry(&xtime_lock, seq)); > > + nsecs += time.tv_sec * 1000000000ULL + time.tv_nsec; > > + return (ktime_t) { .tv64 = nsecs }; > > This will break all 32bit architectures which do not have > CONFIG_KTIME_SCALAR set.
Yep, that is a bug.
> With the above changes applied: > > return ktime_add_ns(ktime_set(secs, 0), nsecs); > > will do the trick. Might need some comments though :)
Ok, will update the patch. Thanks for the review.
-- blue skies, Martin.
"Reality continues to ruin my life." - Calvin.
| |