CODEWITHSUNDEEP
linuxmultithreading
Jun
Jun

Reputation: 426

Race condition on ticket-based ARM spinlock

I found that spinlocks in Linux kernel are all using "ticket-based" spinlock now. However after looking at the ARM implementation of it, I'm confused because the "load-add-store" operation is not atomic at all. Please see the code below:

 74 static inline void arch_spin_lock(arch_spinlock_t *lock)
 75 {
 76         unsigned long tmp;
 77         u32 newval;
 78         arch_spinlock_t lockval;
 79 
 80         __asm__ __volatile__(
 81 "1:     ldrex   %0, [%3]\n"   /*Why this load-add-store is not atomic?*/
 82 "       add     %1, %0, %4\n"
 83 "       strex   %2, %1, [%3]\n"
 84 "       teq     %2, #0\n"
 85 "       bne     1b"
 86         : "=&r" (lockval), "=&r" (newval), "=&r" (tmp)
 87         : "r" (&lock->slock), "I" (1 << TICKET_SHIFT)
 88         : "cc");
 89 
 90         while (lockval.tickets.next != lockval.tickets.owner) {
 91                 wfe();
 92                 lockval.tickets.owner = ACCESS_ONCE(lock->tickets.owner);
 93         }
 94 
 95         smp_mb();
 96 }

As you can see, on line 81~83 it loads lock->slock to "lockval" and increment it by one and then store it back to the lock->slock.

However I didn't see anywhere this is ensured to be atomic. So it could be possible that:

Two users on different cpu are reading lock->slock to their own variable "lockval" at the same time; Then they add "lockval" by one respectively and then store it back.

This will cause these two users are having the same "number" in hand and once the "owner" field becomes that number, both of them will acquire the lock and do operations on some shared-resources!

I don't think kernel can have such a bug in spinlock. Am I wrong somewhere?

Upvotes: 1

Views: 614

Answers (1)

Ben Voigt
Ben Voigt

Reputation: 283684

STREX is a conditional store, this code has Load Link-Store Conditional semantics, even if ARM doesn't use that name.

The operation either completes atomically, or fails.

The assembler block tests for failure (the tmp variable indicates failure) and reattempts the modification, using the new value (updated by another core).

Upvotes: 2

Related Questions