I think you're absolutely right.
On Fri, Nov 20, 2015 at 10:05 AM 'Davide Libenzi' via Akaros <
[email protected]> wrote:
> 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.
>
--
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.
