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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux