Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/dev/hyperv/vmbus/amd64/hyperv_machdep.c | ||
|---|---|---|
| 82 ↗ | (On Diff #22839) | Don't you need volatile read of tsc_seq there ? |
| 96 ↗ | (On Diff #22839) | ... 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 ↗ | (On Diff #22839) | tsc_seq is volatile qualified, so I think the read here is volatile. |
| 96 ↗ | (On Diff #22839) | 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. |
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 ↗ | (On Diff #22839) | Ok. |
| 96 ↗ | (On Diff #22839) | 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. |