Yeah, my brain was clearly in neutral when I looked at that code. ron
On Fri, Nov 20, 2015 at 9:45 AM 'Davide Libenzi' via Akaros < [email protected]> wrote: > It not much even about volatile. Yes, volatile can work, but this is > beyond what volatile is meant for. > > void foo(void) { > volatile int k; > > k = something(); > if (setjmp()) { > A: > use(k); > ... > } > B: > k = k + 9; > C: > something_which_might_longjmps(); > } > > Here A and B, for GCC, are two disjoint domains. > At the time A is possibly executing, B cannot possibly have run, so yes, > the use(k) will reload k from stack storage, but nothing tells GCC to flush > k, a local variable, no more accessed after C, to storage. > The original meaning of volatile, meant, always reload from storage. > I am not sure if adding the "always flush to backing storage after modify, > even if no more users follow", is a wise forward looking assumption. > IMO, on top of volatile, it would be wise not to use, within the waserror > section, variables whose values are written after it. > > > > > On Fri, Nov 20, 2015 at 9:27 AM, Kevin Klues <[email protected]> wrote: > >> 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. >> > > -- > 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. > -- 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.
