Re: [patch 9/9] Make use of the Master Timer

From: Jiri Bohac
Date: Thu Feb 01 2007 - 09:26:41 EST


On Thu, Feb 01, 2007 at 12:36:05PM +0100, Andi Kleen wrote:
> On Thursday 01 February 2007 11:00, jbohac@xxxxxxx wrote:
>
> > + case VXTIME_TSC:
> > + rdtscll(tsc);
>
> Where is the CPU synchronization?
>
> > + cpu = smp_processor_id();
> > + rdtscll(t);
>
> Also no synchronization. It's slower, but needed.

Hmm, I wasn't sure. Why is it needed? How outdated can the
result of RDTSC / RDTSCP be?

If I do:
rdtscll(a)
...
rdtscll(b)
is it guaranteed that (b > a) ?

>
> > unsigned long long sched_clock(void)
> > {
> > - unsigned long a = 0;
> > -
> > - rdtscll(a);
> > - return cycles_2_ns(a);
> > + return monotonic_clock();
> > }
>
> This is overkill because sched_clock() doesn't need a globally monotonic
> clock, per CPU monotonic is enough. The old version was fine.

OK, thanks for spotting this. I'll change it to use __guess_mt().
(more or less equal to cycles_2_ns(), no need to maintain yet another
tsc->ns ratio just for cycles_2_ns().


> > +static __always_inline void do_vgettimeofday(struct timeval * tv, u64 tsc, int cpu)
> > +{
> > + unsigned int sec;
> > + s64 nsec;
> >
> > - do {
> > - sequence = read_seqbegin(&__xtime_lock);
> > -
> > - sec = __xtime.tv_sec;
> > - usec = __xtime.tv_nsec / 1000;
> > -
> > - usec += ((readl((void __iomem *)
> > - fix_to_virt(VSYSCALL_HPET) + 0xf0) -
> > - __vxtime.last) * __vxtime.quot) >> 32;
> > - } while (read_seqretry(&__xtime_lock, sequence));
> > + sec = __xtime.tv_sec;
> > + nsec = __xtime.tv_nsec;
> > + nsec += max(__do_gettimeoffset(tsc, cpu), __vxtime.drift);
> >
> > - tv->tv_sec = sec + usec / 1000000;
> > - tv->tv_usec = usec % 1000000;
> > + sec += nsec / NSEC_PER_SEC;
> > + nsec %= NSEC_PER_SEC;
>
> Using while() here is probably faster (done in vdso patchkit where
> gtod got mysteriously faster). Modulo and divisions are slow, even
> for constants when they are large.

OK, will do that

>
> > }
> >
> > /* RED-PEN may want to readd seq locking, but then the variable should be write-once. */
> > @@ -107,10 +118,39 @@ static __always_inline long time_syscall
> >
> > int __vsyscall(0) vgettimeofday(struct timeval * tv, struct timezone * tz)
> > {
> > - if (!__sysctl_vsyscall)
> > + int cpu = 0;
> > + u64 tsc;
> > + unsigned long seq;
> > + int do_syscall = !__sysctl_vsyscall;
> > +
> > + if (tv && !do_syscall)
> > + switch (__vxtime.mode) {
> > + case VXTIME_TSC:
> > + case VXTIME_TSCP:
> > + do {
> > + seq = read_seqbegin(&__xtime_lock);
> > +
> > + if (__vxtime.mode == VXTIME_TSC)
> > + rdtscll(tsc);
> > + else {
> > + rdtscpll(tsc, cpu);
> > + cpu &= 0xfff;
> > + }
> > +
> > + if (unlikely(__vxtime.cpu[cpu].tsc_invalid))
> > + do_syscall = 1;
> > + else
> > + do_vgettimeofday(tv, tsc, cpu);
> > +
> > + } while (read_seqretry(&__xtime_lock, seq));
> > + break;
> > + default:
> > + do_syscall = 1;
>
> Why do you not set __sysctl_vsyscall correctly for the mode at initialization?

Because of the __vxtime.cpu[cpu].tsc_invalid flag. We may be
using the vsyscall, but when we get the cpufreq PRE- notification, we
know that TSC cannot be trusted from that point on, until the
frequency stabilises. We set the flag and until TSC becomes
reliable again, vsyscall w/ HW Master Timer read will be used.

So this is something that changes in runtime, and cannot be set
permanently on initialization...


--
Jiri Bohac <jbohac@xxxxxxx>
SUSE Labs, SUSE CZ

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/