Re: [PATCH 1/1] Make the TSC safe to be used by gettimeofday().

Suleiman Souhlal
2006-11-15T07:14:32Z

Andi Kleen wrote:
> On Wednesday 15 November 2006 00:36, Suleiman Souhlal wrote:
>
>>This is done by a per-cpu vxtime structure that stores the last TSC and HPET
>>values.
>>
>>Whenever we switch to a userland process after a HLT instruction has been
>>executed or after the CPU frequency has changed, we force a new read of the
>>TSC, HPET and xtime so that we know the correct frequency we have to deal
>>with.
>>
>>We also force a resynch once every second, on every CPU.
>
>
> Hmm, not sure we want to do it this way. Especially since you
> got unsolved races. But patch review ignoring the 10k foot picture again.
>

Which unresolved races?
The reason why I do a resynch every CPU is to prevent the HPET timer from
rolling over more than once before reading it again, which could happen
if the CPU doesn't go into HLT for a long time.

>>+
>>+ /*
>>+ * If we are switching away from a process in vsyscall, touch
>>+ * the vxtime seq lock so that userland is aware that a context switch
>>+ * has happened.
>>+ */
>>+ rip = *(unsigned long *)(prev->rsp0 +
>>+ offsetof(struct user_regs_struct, rip) - sizeof(struct pt_regs));
>>+ if (unlikely(rip > VSYSCALL_START) && unlikely(rip < VSYSCALL_END)) {
>>+ write_seqlock(&vxtime.vx_seq);
>>+ write_sequnlock(&vxtime.vx_seq);
>>+ }
>>+
>
>
> Can't this starve? If a process is unlucky enough (e.g. from
> lots of interrupts) that it can't go through the vsyscall without at
> least one context switch it will never finish. Ok maybe it's an
> unlikely enough livelock, but it still makes me uncomfortable.

I could make it fall back to a full syscall after a certain number of tries.
This, coupled with using a preemption count instead of touching the seq lock
should prevent the livelock (because if we don't touch the seq lock in switch_to
the livelock would not be possible in the full gettimeofday() syscall).
>
>>
>>- .vxtime : AT(VLOAD(.vxtime)) { *(.vxtime) }
>>- vxtime = VVIRT(.vxtime);
>>-
>> .vgetcpu_mode : AT(VLOAD(.vgetcpu_mode)) { *(.vgetcpu_mode) }
>> vgetcpu_mode = VVIRT(.vgetcpu_mode);
>>
>>@@ -119,6 +116,9 @@ #define VVIRT(x) (ADDR(x) - VVIRT_OFFSET
>> .vsyscall_2 ADDR(.vsyscall_0) + 2048: AT(VLOAD(.vsyscall_2)) { *(.vsyscall_2) }
>> .vsyscall_3 ADDR(.vsyscall_0) + 3072: AT(VLOAD(.vsyscall_3)) { *(.vsyscall_3) }
>>
>>+ .vxtime : AT(VLOAD(.vxtime)) { *(.vxtime) }
>>+ vxtime = VVIRT(.vxtime);
>
>
> Why did you move it?

Because otherwise .jiffies (IIRC) collides with .vsyscall_0.

>
>>+long vgetcpu(unsigned *cpu, unsigned *node, struct getcpu_cache *tcache);
>
>
> externs in c code are still forbidden, even if they don't have
> extern

This is just a forward declaration, but I'll get rid of it anyway.

Thanks a lot for the feedback.
I'll try to address these issues and send and a fixed patch.

-- Suleiman