On Thu, Nov 19, 2015 at 8:52 PM, 'Davide Libenzi' via Akaros <[email protected]> wrote: > Didn't the compiler give you a possible uninitialized use (of tsc) at line > 415 of syscall.c?
I didn't get a warning, though looking at the code, I clearly should have. > That longjmp based waserror, the more I think, the more it scares me. > IMO all the local variables accessed on the longjmp return path, should be > made volatile. I've never been 100% comfortable with waserror() myself, but the syscall abort mechanism uses error() under the hood, so I have to use waserror() here in order to detect an abort and compensate for it. I ran what I have by Ron, and he seemed to think it was OK, but it's clear there are problems with accessing the computed tsc value, as you point out below. > No matter how hard you tell GCC to clobber registers and memory. There is no need for a contrived example. If you look at the assembly generated for nanosleep itself, it is clearly wrong. Below is the code inside the waserror() block. It never even tries to do the subtraction of the original tsc (presumably because it decides it's value must be 0 at this point, which is a little weird because I never initialized it to 0). ffffffffc204dee4: 0f 31 rdtsc ffffffffc204dee6: 48 89 d7 mov %rdx,%rdi ffffffffc204dee9: 89 c0 mov %eax,%eax ffffffffc204deeb: 48 8d 75 a8 lea -0x58(%rbp),%rsi ffffffffc204deef: 48 c1 e7 20 shl $0x20,%rdi ffffffffc204def3: 48 09 c7 or %rax,%rdi ffffffffc204def6: e8 c5 45 00 00 callq ffffffffc20524c0 <tsc2timespec> Contrast this with when I make it volatile: ffffffffc204dee4: 0f 31 rdtsc ffffffffc204dee6: 48 8b 4d 90 mov -0x70(%rbp),%rcx ffffffffc204deea: 48 89 d7 mov %rdx,%rdi ffffffffc204deed: 89 c0 mov %eax,%eax ffffffffc204deef: 48 c1 e7 20 shl $0x20,%rdi ffffffffc204def3: 48 8d 75 a8 lea -0x58(%rbp),%rsi ffffffffc204def7: 48 09 c7 or %rax,%rdi ffffffffc204defa: 48 29 cf sub %rcx,%rdi ffffffffc204defd: e8 de 45 00 00 callq ffffffffc20524e0 <tsc2timespec> It clearly loads the tsc value into %rcx and subtracts it. > IMO, all the local variable accessed within a waserror() return path, and > (re)assigned after the waserror(), must be marked as volatile. > Or bad things happen. Whether it's true in general or not, it's clearly true here. So it's good that we went down this path of discussion. However, the better answer in this specific case is to move the read_tsc() call above the waserror() block, as that provides better timing accuracy anyway. When this is done, we get the expected output because the 'returns_twice' attribute on the setjmp()in the waserror() macro provides the required compiler memory barrier for us. > One more thing Kevin about the timing tests. > These can become jittery with high load, if the test run on a node where the > test app is not the only thing running. > Ask me how I know ... (Jenkins build going red at 0300 in the morning) > OTOH some testing needs to be there. What I resorted in the past, was to run > that test at high priority, do the time();sleep(n);time() sequence 5 times, > and take the lowest of the deltas as sleep time. > Not sure we have to care of these cases for Akaros. By default, all of the utests link in the pthread library, which will turn the process into an mcp before the test runs. Since mcps have dedicated access to their cores, it's probably not much of an issue on Akaros to run at "high priority" compared to Linux. That said, running each test 5 times and taking the lowest delta seems reasonable. I've updated the tests accordingly. -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
