Why do we need pushcli() before xchg()?

Question

I was looking at this implementation of spinlocks, and in particular, the acquire and release functions:

void
acquire(struct spinlock *lk)
{
  pushcli(); // disable interrupts to avoid deadlock.
  if(holding(lk))
    panic("acquire");

  // The xchg is atomic.
  // It also serializes, so that reads after acquire are not
  // reordered before it. 
  while(xchg(&lk->locked, 1) != 0)
    ;

  lk->cpu = cpu;
  getcallerpcs(&lk, lk->pcs);
}

// Release the lock.
void
release(struct spinlock *lk)
{
  if(!holding(lk))
    panic("release");

  lk->pcs[0] = 0;
  lk->cpu = 0;

  xchg(&lk->locked, 0);

  popcli();
}

What is the purpose of the pushcli() and popcli() if the xchg function is atomic? Wouldn't atomicity ensure that only one thread is able to change the value of the lock at any instance and not be interrupted? Why do we have to explicitly disable interruptions to prevent deadlocks?

Deadlock could happen if an interrupt handler tried to acquire the lock while it is held by normal code. The interrupt handler would spin in the `while` loop forever because it could never get the lock because the main code execution is suspended so that has no chance to release it. (Assuming single cpu core here.) — Jester, Mar 02 '20 at 22:29

Marco Bonelli · Accepted Answer · 2020-03-03T00:06:11.063

Wouldn't atomicity ensure that only one thread is able to change the value of the lock at any instance and not be interrupted?

Yes, that is not the issue though. The operation ensures atomicity between threads, but interrupt handlers are still a problem.

Interrupts (specifically hardware interrupts) are asynchronous and can happen at any time, meaning that the code could be doing anything at the time the interrupt is generated.

When an interrupt is caught by the CPU, any code that is running is "suspended" and the interrupt handler is entered. Only after the interrupt handler has finished its job, anything else can proceed as normal.

Now, imagine your code needs to acquire() a spinlock, and the function is defined like this:

void acquire(struct spinlock *lk) {
    while(xchg(&lk->locked, 1) != 0)
        ;

    lk->cpu = cpu;
}

If an interrupt occurs any time after the while above (and before a release()), and the interrupt handler needs to acquire the same spinlock, what happens is that it will try to do the same thing: calling acquire() again, entering the while loop. However, since the spinlock is already held, the loop will never exit. The interrupt handler will keep spinning trying to acquire a lock that will never be released, because by definition an interrupt handler must be executed until its end before the CPU can continue doing anything else. This causes a deadlock. There's nothing else left to do than power down and restart the CPU at this point.

That's the reason why disabling interrupts is needed in the code you are showing.

Spinlocks are broken as a means of establishing a mutex between an interrupt service routine and main-line code. If interrupts are going to be disabled while a lock is held, however, what's the need for the lock? — supercat, Mar 02 '20 at 23:55
@supercat interrupts are definitely not the only thing that could try accessing the resource. Locks are needed to coordinate multiple *threads* accessing the same resource. The fact that interrupts can cause trouble is a secondary problem. — Marco Bonelli, Mar 02 '20 at 23:56
Disabling interrupts on a core will disable most but not all forms of thread-switching on that core, and seems like a really bad code smell. The proper approach I would think would be to require that interrupt handlers never spin-lock on a mutex, but instead set things up so they'll get retriggered once the mutex becomes available and then abort their operation unless/until that happens. — supercat, Mar 03 '20 at 00:03
@supercat well good to know, but that is not in the scope of the question. — Marco Bonelli, Mar 03 '20 at 00:05

Why do we need pushcli() before xchg()?

1 Answers1