Re: [PATCH] lock: Add doc comments for spin_lock_irq()

Waiman Long
2025-10-12T23:17:57Z

On 10/12/25 9:48 AM, Daroc Alden wrote:
> On Sat, 11 Oct 2025 22:31:17 -0400
> Waiman Long <llong@redhat.com> wrote:
>
>> On 10/11/25 2:28 PM, Daroc Alden wrote:
>>> On Fri, 10 Oct 2025 23:15:50 -0400
>>> Waiman Long <llong@redhat.com> wrote:
>>>
>>>> On 10/10/25 5:53 PM, Daroc Alden wrote:
>>>>> The commonly used spin_lock_irq(), spin_lock_irqsave(),
>>>>> spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
>>>>> currently have any documentation; this commit adds kerneldoc
>>>>> comments to these four functions describing when their behavior
>>>>> and when they are appropriate to use.
>>>>>
>>>>> Signed-off-by: Daroc Alden <daroc@lwn.net>
>>>> This patch looks fine. Just wonder why just
>>>> spin_lock_irq()/spin_lock_irqsave() and not
>>>> spin_lock()/spin_lock_bh() as these functions also don't have
>>>> kerneldoc comments. Also spin_lock_irqsave() is a macro and not
>>>> actually a function, maybe we should mention that in the comment.
>>>>
>>> Because I had to research spin_lock_irq()/spin_lock_irqsave() for a
>>> recent article, and therefore felt confident that I understood how
>>> they behaved and what should go in the doc comment.
>>>
>>> If you — as a more experienced kernel person — can describe how/why
>>> the _bh() variants are used, I'm happy to add doc comments for them
>>> as well. My current understanding is that they interact with
>>> whatever is left of the "big kernel lock". Is that right?
>> "bh" in spin_lock_bh() stands for bottom half which is essentially
>> what what is being done in the softIRQ context. So spin_lock_bh()
>> just prevents the softIRQ code from being executed. This is my
>> understanding, but I may have missed other use cases of
>> spin_lock_bh(). Others can chime in if there is more to say. Anyway,
>> I am fine with adding more comments to spinlock code.
>>
> Ah, okay!
>
> I went and read some of the existing locking documentation with that
> context in mind, and I think I understand. I think the doc comment
> should look something like this:
>
> /**
> * spin_lock_bh() - Disable softIRQs and take the provided spinlock.
> * @lock: The spinlock to acquire.
> *
> * When data is shared between code that can run in process context and
> * code that can run in a softIRQ, if the softIRQ tries to acquire a
> * spinlock that is already held, the system could deadlock. This
> * function disables softIRQs before taking the provided spinlock. It
> * should typically be paired with a call to spin_unlock_bh() in order
> * to reenable softIRQs when the lock is released.
> *
> * If the interrupt code can run as a hard interrupt instead of a soft
> * interrupt, this is the wrong function: use spin_lock_irqsave(). If in
> * doubt, using spin_lock_irqsave() instead of spin_lock_bh() is always
> * permissible, since the former is a superset of the latter.
I believe the above description is correct.
> *
> * If synchronizing between a tasklet or timer and a softIRQ, the plain
> * spin_lock() function can be used, because these are not interrupted
> * by softIRQs on the same CPU.
> */

Tasklets and timer handling are run in softIRQ context.  SoftIRQs are
sub-divided into a number of priority levels (see
include/linux/interrupt.h) from high to low, they are not going to
interrupt each other.

Cheers,
Longman