Details

Reviewers

Commits

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

sepherosa_gmail.com retitled this revision from to hyperv: Implement "enlightened" time counter, which is rdtsc based..

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

Fallback to other timecounter's tc_get_timecount is not a good idea.

Fix reversed logic

sys/dev/hyperv/vmbus/amd64/hyperv_machdep.c
82	Don't you need volatile read of tsc_seq there ?
96	... and there ? Also, I believe that you want to tsc_scale and tsc_ofs reads to occur not earlier than tsc_seq read occured. x86 is strongly ordered WRT reads, but compiler is not. It seems that you want tsc_seq in the while () condition to occur using atomic_load_acq(), and you need atomic_thread_fence_acq() barrier before this re-read.

sys/dev/hyperv/vmbus/amd64/hyperv_machdep.c
82	tsc_seq is volatile qualified, so I think the read here is volatile.
96	I considered about adding explicit compiler_membar though. As about the atomic_..._acq functions, IIRC, some said atomic_..._acq normally should be paired w/ the atomic_..._rel, while obviously the atomic_..._rel is on the hypervisor side. Do you think it's fine to use atomic_..._acq w/o atomic_..._rel? I am absolutely fine w/ using the atomic_..._acq.

Add compiler fence through atomic_*_acq, suggested by kib

Looks good for me, as far as I do not know/understand the HV interface proper.

sys/dev/hyperv/vmbus/amd64/hyperv_machdep.c
82	Ok.
96	It was me who made that note, among others. Point is, if implementation of atomic(9) changes, this should be reconsidered. I am fine with the use of __compiler_membar() if you prefer that. Note that right now memory model details of hyperv_tsc_timecount() are identical to that of libc/kernel binuptime() function.

This revision is now accepted and ready to land.Dec 13 2016, 11:19 AM

This revision was automatically updated to reflect the committed changes.