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

[Daroc Alden <daroc@lwn.net>]

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.
 *
 * 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.
 */

If nobody chimes in to correct me, I'll add that (and a similar
comment for the plain spin_lock()) and post a v2 in a day or two.


Thanks for the help!

-- 
Daroc Alden (they/them)
Editor, LWN | https://lwn.net