Let me rephrase my last statement:
"IMO, on top of volatile, it would be wise not to assume, within the
waserror section, that the values written into variables after it, will
stick."
IOW, the code within a waserror section should be as dumb as possible,
using only assign-once variables whose assign happened before the waserror
using them.
Typical example:
{
res r;
r = res_alloc();
if (waserror()) {
res_free(r);
nexterror();
}
// from here 'r' is to be assumed read only
}
On Fri, Nov 20, 2015 at 9:53 AM, ron minnich <[email protected]> wrote:
> 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.
>
--
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.
