Message-ID: <9a2ddd23-9c19-48d7-b58b-0c74b4777563@redhat.com>
Date: Sun, 12 Oct 2025 19:17:51 -0400
From: Waiman Long <llong@...hat.com>
To: Daroc Alden <daroc@....net>, Waiman Long <llong@...hat.com>
Cc: corbet@....net, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
 Boqun Feng <boqun.feng@...il.com>,
 "open list:LOCKING PRIMITIVES" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lock: Add doc comments for spin_lock_irq()

On 10/12/25 9:48 AM, Daroc Alden wrote:
> On Sat, 11 Oct 2025 22:31:17 -0400
> Waiman Long <llong@...hat.com> wrote:
>
>> On 10/11/25 2:28 PM, Daroc Alden wrote:
>>> On Fri, 10 Oct 2025 23:15:50 -0400
>>> Waiman Long <llong@...hat.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@....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

Powered by blists - more mailing lists