lkmpg icon indicating copy to clipboard operation
lkmpg copied to clipboard

Correct the descriptions about `spin_lock_irqsave` and `spin_unlock_irqrestore`

Open jserv opened this issue 3 years ago • 2 comments

__next__ at Reddit gave the feedback:

Interrupts can't happen during the lock as the main idea of using spin_lock_irq is to disable (on the logical core, let's use "local CPU" term for that) interrupts.

The whole idea of using spin_lock_irqsave instead of just spin_lock_irq is what is happening during the unlock. At the time spin_lock_irq() is used, interrupts could have been already disabled on the local CPU. And now, when we use spin_unlock_irq() it will re-enable interrupts again for the whole (local) CPU! Yes, it means that our code could have enabled interrupts while someone else (who disabled them before we took the lock) is not expecting that. Example to visualize:

  1. Interrupts are disabled on a local CPU,
  2. We use spin_lock_irq() -> interrupts disabled on a local CPU,
  3. We use spin_unlock_irq() -> interrupts enabled on a local CPU

So in point 3. all interrupts were enabled (on the local CPU) and the one who disabled them in point 1. is not aware of that fact at all. We enabled the interrupts for him. _irqsave variants were introduced to solve this problem.

  1. Interrupts are disabled on a local CPU,
  2. We use spin_lock_irqsave(spinlock_t *lock, unsigned long flags) -> we know that interrupts were disabled at this point as we saved the current state of them in the flags parameter,
  3. We use spin_unlock_irqrestore -> and at this point, we know based on the flags value, that the interrupts were disabled before _irqsave, so we just go back to the state before invoking spin_lock_irqsave instead of always re-enabling interrupts like in the case with spin_unlock_irq().

jserv avatar Aug 16 '21 17:08 jserv

I don't think I have got what all means in __next__'s comment "all interrupts were enabled". The CPU has a set of the exceptions one of which is IRQ. And enabling IRQ exception on the CPU side is what's done there. Besides that NMI is by definition can't be handled by this.

ashevche avatar Feb 21 '23 14:02 ashevche

I think the idea __next__ wants to talk about is the CPU may unconditionally enable the interrupts, including those already disabled before the spin_lock_irq().

linD026 avatar Feb 22 '23 08:02 linD026