0

I was reading a section of kernel/timer.c file and came across this section which uses READ_ONCE and a comment which states that using READ_ONCE() prevents multiple reads by the compiler.

/*
         * We need to use READ_ONCE() here, otherwise the compiler
         * might re-read @tf between the check for TIMER_MIGRATING
         * and spin_lock().
         */
        tf = READ_ONCE(timer->flags);

        if (!(tf & TIMER_MIGRATING)) {
            base = get_timer_base(tf);
            raw_spin_lock_irqsave(&base->lock, *flags);
            if (timer->flags == tf)
                return base;

I then found this question on SO: Can a compiler read twice from a global variable, instead of storing a local one?

which tells, that the compiler will indeed read the global variable multiple times. However, in this case, a shared global variable is assigned to a local non volatile variable. As per this question: $tf can also not be elided. So since they are assigning a volatile variable (flags) to a non-volatile variable ($tf), does the properties of volatile transfer to a non-volatile variable ($tf) as well, so that $tf can also be prevented from re-reads ?

Haswell
  • 1,573
  • 1
  • 18
  • 45
  • `So since they are assigning a volatile variable (flags)`. `flags` is not volatile. `flags` a normal `u32`. – KamilCuk Dec 15 '20 at 11:45
  • #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x)) – Haswell Dec 15 '20 at 12:25
  • 2
    Yes, but _flags_ is not volatile. If you would do `tf = timer->flags` then the compiler could have read it multiple times. I do not understand, it's not a problem if `tf` is read multiple times, it has the same constant value. It's a problem to read `timer->flags` multiple times, because it may be asynchronously modified by another thread between two reads. – KamilCuk Dec 15 '20 at 13:08
  • @KamilCuk Yes, my question arose from that comment in the code, also if you go to the timer.c link, they have an __acquire(timer->lock) taken, I don't know if it actually takes the lock, because if it does, then there is no chance timer->flags will be modified else where. – Haswell Dec 15 '20 at 13:18
  • 1
    No it doesn't. That's completely something else. It's used for analysis of kernel code. And it's before `{` `}`, not in function body! – KamilCuk Dec 15 '20 at 13:43

2 Answers2

1

volatile variables have to be read as many times as they are used. Non-volatile variables do not have to (if you enable the optimizations).

volatile int x;
int y;

int foo(void)
{
    return x + x + x;
}

int bar(void)
{
    return y + y + y;
}

foo:
        mov     eax, DWORD PTR x[rip]
        mov     ecx, DWORD PTR x[rip]
        mov     edx, DWORD PTR x[rip]
        add     eax, ecx
        add     eax, edx
        ret
bar:
        mov     eax, DWORD PTR y[rip]
        lea     eax, [rax+rax*2]
        ret

https://godbolt.org/z/P4aT47

0___________
  • 60,014
  • 4
  • 34
  • 74
  • Thanks for the compiler playground link, I checked below code in the link, 1st without const volatile *, https://godbolt.org/z/nKG96r and next with const volatile *, https://godbolt.org/z/vM55MM, the earlier code skips the comparison, so the $ts also behaves as a volatile – Haswell Dec 15 '20 at 12:49
1

So since they are assigning a volatile variable (flags)

flags is not volatile. It is accesssed using a volatile access. Note that "volatile" is keyword that disallows compiler optimizations - nothing more, nothing less.

to a non-volatile variable ($tf), does the properties of volatile transfer to a non-volatile variable ($tf) as well, so that $tf can also be prevented from re-reads ?

No, tf is a normal variable, it may be read or written to multiple times. There is no such thing as "transferring properties".

If you would write:

   u32 tf;
   tf = timer->flags;
    if (!(tf & TIMER_MIGRATING)) {
        base = get_timer_base(tf);
        raw_spin_lock_irqsave(&base->lock, *flags);
        if (timer->flags == tf)
            return base;

then the variable tf could be optimized out and the code could be optimized to:

    if (!(timer->flags & TIMER_MIGRATING)) {
        // another thread may asynchronously modify timer->flags here
        base = get_timer_base(timer->flags);
        raw_spin_lock_irqsave(&base->lock, *flags);
        // save here - we are in irqsave section
        if (timer->flags == timer->flags) // could be optimized further...
            return base;

Because the operation on timer->flags is done via a volatile qualified handle, it can't be optimized, it has to happen exactly where you want it - only one time, before anything else and all side effects (ie. other volatile writes and reads) before the check have to be completed.

KamilCuk
  • 120,984
  • 8
  • 59
  • 111