runtime: use vDSO page for time on FreeBSD/amd64.

runtime: use vDSO page for time on FreeBSD/amd64. This is similar in spirit to the Linux work done in https://codereview.appspot.com/6454046/ Using this test http://play.golang.org/p/QUwb_r1d0G Performance of go tip vs this patch with 'sysctl kern.timecounter.fast_gettime=0': benchmark old ns/op new ns/op delta BenchmarkStdTime 277 283 +2.17% Performance of this patch with 'sysctl kern.timecounter.fast_gettime=0' vs 'sysctl kern.timecounter.fast_gettime=1': benchmark old ns/op new ns/op delta BenchmarkStdTime 283 37.3 -86.82%

[Ivan Krasin, 2014-09-30 17:51:33]

Hi Bill, there's a few initial comments. Let's start with them. I would be grateful if you test the changes I proposed. Unfortunately, I don't know of any good way to test it on FreeBSD. https://codereview.appspot.com/146530043/diff/20001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:153: TEXT rdtsc(SB), NOSPLIT, $0 This seems to be a duplicate of runtime·cputicks, see src/runtime/asm_amd64.s If so, could you please use runtime·cputicks directly and remove this function? https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:3: // license that can be found in the LICENSE file. There's important difference between the VDSO code provided by Linux (which is Creative Commons Zero License, http://creativecommons.org/publicdomain/zero/1.0/legalcode, it's essentially public domain), and the FreeBSD code (https://github.com/freebsd/freebsd/blob/master/lib/libc/amd64/sys/__vdso_gettc.c) which is provided under 2-clause BSD license. I will left the decision on how to actually formalize the header in this file to comply with the license to the real Go authors, but there's something needs to be done for sure. https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:78: return rdtsc() >> th->th_x86_shift; Yes, you're right. It's just a right shift. Feel free to remove the comment. https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:86: return (th->th_x86_shift > 0 ? __vdso_gettc_low(th) : rdtsc32()); it seems that rdtsc32 is just runtime·cputicks() & 0xFFFF. Is it? https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:134: if(tk->tk_enabled == 0) { it seems that these two checks (tk_enabled and tk_ver) could be made at the parse time, instead of on each time·now call. If the check fails, just assign 0 to runtime·__vdso_timekeep_sym and then each time·now would become simpler and faster. Please, correct me, if I made a wrong assumption.

[wathiede, 2014-09-30 19:32:21]

https://codereview.appspot.com/146530043/diff/20001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:153: TEXT rdtsc(SB), NOSPLIT, $0 On 2014/09/30 17:51:33, Ivan Krasin wrote: > This seems to be a duplicate of runtime·cputicks, see src/runtime/asm_amd64.s > > If so, could you please use runtime·cputicks directly and remove this function? I knew there had to be something better. https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:3: // license that can be found in the LICENSE file. On 2014/09/30 17:51:33, Ivan Krasin wrote: > There's important difference between the VDSO code provided by Linux (which is > Creative Commons Zero License, > http://creativecommons.org/publicdomain/zero/1.0/legalcode, it's essentially > public domain), and the FreeBSD code > (https://github.com/freebsd/freebsd/blob/master/lib/libc/amd64/sys/__vdso_gettc.c) > which is provided under 2-clause BSD license. > > I will left the decision on how to actually formalize the header in this file to > comply with the license to the real Go authors, but there's something needs to > be done for sure. ACK https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:78: return rdtsc() >> th->th_x86_shift; On 2014/09/30 17:51:33, Ivan Krasin wrote: > Yes, you're right. It's just a right shift. Feel free to remove the comment. Done. https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:86: return (th->th_x86_shift > 0 ? __vdso_gettc_low(th) : rdtsc32()); On 2014/09/30 17:51:33, Ivan Krasin wrote: > it seems that rdtsc32 is just runtime·cputicks() & 0xFFFF. Is it? /usr/src/sys/amd64/include/cpufunc.h says: static __inline uint64_t rdtsc(void) { uint32_t low, high; __asm __volatile("rdtsc" : "=a" (low), "=d" (high)); return (low | ((uint64_t)high << 32)); } static __inline uint32_t rdtsc32(void) { uint32_t rv; __asm __volatile("rdtsc" : "=a" (rv) : : "edx"); return (rv); } So I think the answer is yes, although I'm not very good at reading asm. I've updated the code assuming it is. Also, I only had the rdtsc{32} functions in assembly because I didn't know how to perform a RDTSC in C. Now that you've pointed me at runtime.cputicks I've removed some function calls here. https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:134: if(tk->tk_enabled == 0) { On 2014/09/30 17:51:33, Ivan Krasin wrote: > it seems that these two checks (tk_enabled and tk_ver) could be made at the > parse time, instead of on each time·now call. If the check fails, just assign 0 > to runtime·__vdso_timekeep_sym and then each time·now would become simpler and > faster. > > Please, correct me, if I made a wrong assumption. I had thought that too. tk_enable could change if someone runs sysctl kern.timecounter.fast_gettime=0 or 1, at the very least it must be checked once per time.now. I'm not sure about the tk_ver and th_algo values. I followed libc's lead on this. I think the timehand structures can have their backing clock changed with sysctl kern.timecounter.hardware & kern.timecounter.choice. I'm not sure if the version and algorithm will change with that, I'll research more.

[Ivan Krasin, 2014-09-30 20:29:50]

https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:134: if(tk->tk_enabled == 0) { On 2014/09/30 19:32:21, wathiede wrote: > On 2014/09/30 17:51:33, Ivan Krasin wrote: > > it seems that these two checks (tk_enabled and tk_ver) could be made at the > > parse time, instead of on each time·now call. If the check fails, just assign > 0 > > to runtime·__vdso_timekeep_sym and then each time·now would become simpler and > > faster. > > > > Please, correct me, if I made a wrong assumption. > > I had thought that too. tk_enable could change if someone runs sysctl > kern.timecounter.fast_gettime=0 or 1, at the very least it must be checked once > per time.now. I'm not sure about the tk_ver and th_algo values. I followed > libc's lead on this. I think the timehand structures can have their backing > clock changed with sysctl kern.timecounter.hardware & kern.timecounter.choice. > I'm not sure if the version and algorithm will change with that, I'll research > more. In this case, I would ask somebody relevant to FreeBSD kernel. My suggestion: add iant@golang.org as the reviewer. If he is available, he is the best person to review this CL from the Go team side. rsc@golang.org is another good option. Also, once the copyright issue is sorted out, I would recommend to kindly ask Konstantin Belousov <kib@FreeBSD.ORG> to review the semantics of the code. Last time, I didn't have a courage to ask Andy Lutomirski to take a look at my Linux VDSO CL and that was a mistake that was about to become spectacular. Only the preventive move from Andy saved all Go programs to start crashing on a newer Linux kernels. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:162: CALL	runtime·__vdso_timekeep(SB) I think that this function could be made simpler. Instead of filling bintime structure in __vdso_timekeep and then parsing the results here, I would recommend to return the same value as time·now. Anyway, there's no option to avoid C in this call, given the complexity of __vdso_timekeep implementation. In this case, time·now will look like: MOVQ runtime·__vdso_timekeep_sym(SB), AX CMPQ AX, $0 JEQ fallback_gtod CALL runtime·__vdso_timekeep(SB) CMPL AX, $0 // kern.timecounter.fast_gettime=0 or there is some kernel version skew, // fallback to slower syscall. JEQ fallback_syscall RET fallback_syscall: ...

[wathiede, 2014-09-30 22:35:41]

https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/20001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:134: if(tk->tk_enabled == 0) { On 2014/09/30 20:29:50, Ivan Krasin wrote: > On 2014/09/30 19:32:21, wathiede wrote: > > On 2014/09/30 17:51:33, Ivan Krasin wrote: > > > it seems that these two checks (tk_enabled and tk_ver) could be made at the > > > parse time, instead of on each time·now call. If the check fails, just > assign > > 0 > > > to runtime·__vdso_timekeep_sym and then each time·now would become simpler > and > > > faster. > > > > > > Please, correct me, if I made a wrong assumption. > > > > I had thought that too. tk_enable could change if someone runs sysctl > > kern.timecounter.fast_gettime=0 or 1, at the very least it must be checked > once > > per time.now. I'm not sure about the tk_ver and th_algo values. I followed > > libc's lead on this. I think the timehand structures can have their backing > > clock changed with sysctl kern.timecounter.hardware & kern.timecounter.choice. > > > I'm not sure if the version and algorithm will change with that, I'll research > > more. > > In this case, I would ask somebody relevant to FreeBSD kernel. > > My suggestion: add mailto:iant@golang.org as the reviewer. If he is available, he is > the best person to review this CL from the Go team side. mailto:rsc@golang.org is > another good option. > > Also, once the copyright issue is sorted out, I would recommend to kindly ask > Konstantin Belousov <mailto:kib@FreeBSD.ORG> to review the semantics of the code. > > Last time, I didn't have a courage to ask Andy Lutomirski to take a look at my > Linux VDSO CL and that was a mistake that was about to become spectacular. Only > the preventive move from Andy saved all Go programs to start crashing on a newer > Linux kernels. Good advice. I will follow up with kib@freebsd.org after iant@ gets a look at this CL. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:162: CALL	runtime·__vdso_timekeep(SB) On 2014/09/30 20:29:50, Ivan Krasin wrote: > I think that this function could be made simpler. Instead of filling bintime > structure in __vdso_timekeep and then parsing the results here, I would > recommend to return the same value as time·now. Anyway, there's no option to > avoid C in this call, given the complexity of __vdso_timekeep implementation. > > In this case, time·now will look like: > MOVQ runtime·__vdso_timekeep_sym(SB), AX > CMPQ AX, $0 > JEQ fallback_gtod > > CALL runtime·__vdso_timekeep(SB) > CMPL AX, $0 > // kern.timecounter.fast_gettime=0 or there is some kernel version skew, > // fallback to slower syscall. > JEQ fallback_syscall > RET > > fallback_syscall: > ... Okay, naive question, what does the C function look like to return (sec int64, nsec int32)? is it: int32 runtime·__vdso_timekeep(int64 *sec, int32 *nsec); And then I deference and assign in C like: *sec = <some seconds value>; *nsec = <some nanoseconds value>; return enabled;

[Ivan Krasin, 2014-09-30 22:46:36]

https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:162: CALL	runtime·__vdso_timekeep(SB) On 2014/09/30 22:35:41, wathiede wrote: > On 2014/09/30 20:29:50, Ivan Krasin wrote: > > I think that this function could be made simpler. Instead of filling bintime > > structure in __vdso_timekeep and then parsing the results here, I would > > recommend to return the same value as time·now. Anyway, there's no option to > > avoid C in this call, given the complexity of __vdso_timekeep implementation. > > > > In this case, time·now will look like: > > MOVQ runtime·__vdso_timekeep_sym(SB), AX > > CMPQ AX, $0 > > JEQ fallback_gtod > > > > CALL runtime·__vdso_timekeep(SB) > > CMPL AX, $0 > > // kern.timecounter.fast_gettime=0 or there is some kernel version skew, > > // fallback to slower syscall. > > JEQ fallback_syscall > > RET > > > > fallback_syscall: > > ... > > Okay, naive question, what does the C function look like to return (sec int64, > nsec int32)? > > is it: > > int32 runtime·__vdso_timekeep(int64 *sec, int32 *nsec); > > And then I deference and assign in C like: > > *sec = <some seconds value>; > *nsec = <some nanoseconds value>; > return enabled; I guess, there's a way, since it's a very special flavour of C, but I don't know the answer. Let's add Ian Lance Taylor (iant@golang.org) to the thread.

[Ivan Krasin, 2014-09-30 22:49:46]

Hi Ian Lance, Bill has been working on vDSO support for FreeBSD and he has reached the stage when it kind of works. Since vDSO is a tricky bit, it would be useful to get an early feedback from you (and maybe Russ). After that, the plan is to ask the FreeBSD vDSO maintainer to take a look as well. Any comments are appreciated.

[iant, 2014-10-01 00:22:14]

https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:151: // TODO(reviewer): I have no idea what the correct value for $32 is. I just changed the number until it worked. Please help. $32 is the number of bytes that the function allocates on the stack. Try tweaking it and using a disassembler to look at the resulting code in the final program. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:160: LEAQ	16(SP),CX This is the value you are passing for the parameter to __vdso_timekeep. You need the address of a 16 byte area. Reserving 32 bytes of stack space, and starting at an offset of 16, gives you that. So you are good. The function needs 12 bytes of stack space: 8 bytes for the parameter, and 4 bytes for the return value. So, again, at an offset of 16, you are good. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:163: CMPL	AX, $0 This looks wrong. C functions now return values on the stack, not in AX. This was a recent change. Are you testing on tip? https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:168: MOVQ	16(SP), BX	// bintime.sec - whole seconds Why bother to do this in assembler code? You're calling C anyhow; why not do this in the C function? https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:131: * XXXKIB. The load of tk->tk_current should use This comment isn't really meaningful for the Go runtime, where there is no atomic_load_acq_32. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:136: curr = tk->tk_current; Use runtime_atomicload. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:144: gen = th->th_gen; Use runtime_atomicload. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:154: } while(curr != tk->tk_current || gen == 0 || gen != th->th_gen); Use runtime_atomicload for tk->tk_current and th->th_gen. Then you can drop the comments about rmb and lfence. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:163: Delete this blank line.

[iant, 2014-10-01 00:22:49]

I added some comments on the CL. Ian On Tue, Sep 30, 2014 at 3:49 PM, <krasin@golang.org> wrote: > Hi Ian Lance, > > Bill has been working on vDSO support for FreeBSD and he has reached the > stage when it kind of works. > > Since vDSO is a tricky bit, it would be useful to get an early feedback > from you (and maybe Russ). After that, the plan is to ask the FreeBSD > vDSO maintainer to take a look as well. > > Any comments are appreciated. > > https://codereview.appspot.com/146530043/ 

[wathiede, 2014-10-01 04:51:45]

I will request you PTAL when I implement all the comments. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:151: // TODO(reviewer): I have no idea what the correct value for $32 is. I just changed the number until it worked. Please help. On 2014/10/01 00:22:13, iant wrote: > $32 is the number of bytes that the function allocates on the stack. Try > tweaking it and using a disassembler to look at the resulting code in the final > program. Acknowledged. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:163: CMPL	AX, $0 On 2014/10/01 00:22:13, iant wrote: > This looks wrong. C functions now return values on the stack, not in AX. This > was a recent change. Are you testing on tip? I've been building on: parent: 21314:8f36a32a9d03 tip crypto/tls: ensure that we don't resume when tickets are disabled. I see a serious performance change when I run a test after running 'sysctl kern.timecounter.fast_gettime=0' presumably because tk->tk_enable returns 0 and the JEQ takes us to the fallback_syscall. I'll update to tip again, and modify my code to get the value off the stack. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:168: MOVQ	16(SP), BX	// bintime.sec - whole seconds On 2014/10/01 00:22:13, iant wrote: > Why bother to do this in assembler code? You're calling C anyhow; why not do > this in the C function? That was Ivan's suggestion too. I will do that. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:131: * XXXKIB. The load of tk->tk_current should use On 2014/10/01 00:22:14, iant wrote: > This comment isn't really meaningful for the Go runtime, where there is no > atomic_load_acq_32. Acknowledged. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:136: curr = tk->tk_current; On 2014/10/01 00:22:13, iant wrote: > Use runtime_atomicload. Done. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:144: gen = th->th_gen; On 2014/10/01 00:22:14, iant wrote: > Use runtime_atomicload. Done. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:154: } while(curr != tk->tk_current || gen == 0 || gen != th->th_gen); On 2014/10/01 00:22:14, iant wrote: > Use runtime_atomicload for tk->tk_current and th->th_gen. > > Then you can drop the comments about rmb and lfence. Done. https://codereview.appspot.com/146530043/diff/40001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:163: On 2014/10/01 00:22:14, iant wrote: > Delete this blank line. Done.

[wathiede, 2014-10-07 03:39:44]

I have updated the CL to address iant@'s comments. I have also emailed the FreeBSD implementor responsible for this code, kib@, and requested he provide some feedback. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... File src/runtime/sys_freebsd_amd64.s (right): https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:163: CMPL	AX, $0 On 2014/10/01 04:51:45, wathiede wrote: > On 2014/10/01 00:22:13, iant wrote: > > This looks wrong. C functions now return values on the stack, not in AX. > This > > was a recent change. Are you testing on tip? > > I've been building on: > > parent: 21314:8f36a32a9d03 tip > crypto/tls: ensure that we don't resume when tickets are disabled. > > I see a serious performance change when I run a test after running 'sysctl > kern.timecounter.fast_gettime=0' presumably because tk->tk_enable returns 0 and > the JEQ takes us to the fallback_syscall. I'll update to tip again, and modify > my code to get the value off the stack. Done. https://codereview.appspot.com/146530043/diff/40001/src/runtime/sys_freebsd_a... src/runtime/sys_freebsd_amd64.s:168: MOVQ	16(SP), BX	// bintime.sec - whole seconds On 2014/10/01 04:51:45, wathiede wrote: > On 2014/10/01 00:22:13, iant wrote: > > Why bother to do this in assembler code? You're calling C anyhow; why not do > > this in the C function? > > That was Ivan's suggestion too. I will do that. Done.

[iant, 2014-10-09 04:01:32]

https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:6: // obviously borrows code (which I cite) from FreeBSD's libc implementation Perhaps I am wrong, but it seems to me that not much of the code is an exact copy. If there is no exact copy other than things like numeric constants, then I don't think an additional copyright header is required. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:15: // /usr/src/lib/libc/sys/__vdso_gettimeofday.c Give a URL if possible. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:35: // From /usr/include/sys/time.h I would say <sys/time.h> https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:40: // From /usr/include/sys/vdso.h Add a newline before this comment. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:40: // From /usr/include/sys/vdso.h <sys/vdso.h> https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:49: // From /usr/include/x86/vdso.h <x86/vdso.h> (I guess) https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:64: extern int32 runtime·__vdso_timekeep_abs(int64 *sec, int32 *nsec); Is there a reason to declare these functions? https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:70: uint32 This should be static. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:82: // From /usr/include/sys/time.h <sys/time.h> and so on below. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:84: void static https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:97: void static https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:110: int32 static https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:163: runtime·__vdso_timekeep_rel(int64 *nsec) Nothing calls this function. Why do you want it?

[wathiede, 2014-10-19 00:50:29]

PTAL https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:6: // obviously borrows code (which I cite) from FreeBSD's libc implementation On 2014/10/09 04:01:32, iant wrote: > Perhaps I am wrong, but it seems to me that not much of the code is an exact > copy. If there is no exact copy other than things like numeric constants, then > I don't think an additional copyright header is required. Acknowledged. I'll wait until kib@ chimes in to make sure the original author concurs. He said he wouldn't be available to review until after Oct. 23, so I will ping him again then. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:15: // /usr/src/lib/libc/sys/__vdso_gettimeofday.c On 2014/10/09 04:01:32, iant wrote: > Give a URL if possible. Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:35: // From /usr/include/sys/time.h On 2014/10/09 04:01:32, iant wrote: > I would say <sys/time.h> Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:40: // From /usr/include/sys/vdso.h On 2014/10/09 04:01:32, iant wrote: > <sys/vdso.h> Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:40: // From /usr/include/sys/vdso.h On 2014/10/09 04:01:32, iant wrote: > <sys/vdso.h> Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:49: // From /usr/include/x86/vdso.h On 2014/10/09 04:01:31, iant wrote: > <x86/vdso.h> (I guess) Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:64: extern int32 runtime·__vdso_timekeep_abs(int64 *sec, int32 *nsec); On 2014/10/09 04:01:32, iant wrote: > Is there a reason to declare these functions? I forgot to draw your attention to it in the last round of patches, but I created two functions, one that implements the fast path for time.now, and one that implements the fast path for runtime.nanotime. These two functions make it easy to call runtime.vdso_timekeep depending on what you need. The difference is in the arg abs. My understanding of time in FreeBSD is, when abs == 1, then the bootime is added to time counter, yielding a value suitable for a realtime clock. If abs == 0, the value returned is suitable for implementing a monotonic clock. Large time corrections, like daylight savings adjustments, happen by the kernel adjusting the value of boottime. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:70: uint32 On 2014/10/09 04:01:32, iant wrote: > This should be static. Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:82: // From /usr/include/sys/time.h On 2014/10/09 04:01:31, iant wrote: > <sys/time.h> and so on below. Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:84: void On 2014/10/09 04:01:32, iant wrote: > static Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:97: void On 2014/10/09 04:01:32, iant wrote: > static Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:110: int32 On 2014/10/09 04:01:32, iant wrote: > static Done. https://codereview.appspot.com/146530043/diff/60001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:163: runtime·__vdso_timekeep_rel(int64 *nsec) On 2014/10/09 04:01:32, iant wrote: > Nothing calls this function. Why do you want it? See the newly added fast path I added to runtime.nanotime in sys_freebsd_amd64.s. Apologies for not explicitly calling your attention to it in the last round of reviews.

[iant, 2014-10-21 16:19:04]

Ping when you are ready for a final review. https://codereview.appspot.com/146530043/diff/80001/src/runtime/vdso_freebsd_... File src/runtime/vdso_freebsd_amd64.c (right): https://codereview.appspot.com/146530043/diff/80001/src/runtime/vdso_freebsd_... src/runtime/vdso_freebsd_amd64.c:1: // Copyright 2012 The Go Authors. All rights reserved. s/2012/2014/