On Tue, Jun 27, 2023 at 10:34:04AM +0000, Huang, Kai wrote: > On Tue, 2023-06-27 at 12:50 +0300, kirill.shutemov@xxxxxxxxxxxxxxx wrote: > > On Tue, Jun 27, 2023 at 02:12:37AM +1200, Kai Huang wrote: > > > + /* > > > + * The TDX module global initialization only needs to be done > > > + * once on any cpu. > > > + */ > > > + raw_spin_lock_irqsave(&tdx_global_init_lock, flags); > > > > I don't understand how the comment justifies using raw spin lock. > > > > This comment is for using lock in general. The reason to use raw_ version is > because this function gets called in IRQ context, and for PREEMPT_RT kernel the > normal spinlock is converted to sleeping lock. Sorry, but this still doesn't explain anything. Why converting to sleeping lock here is wrong? There are plenty spin_lock_irqsave() users all over the kernel that are fine to be converted to sleeping lock on RT kernel. Why this use-case is special enough to justify raw_? >From the documentation: raw_spinlock_t is a strict spinning lock implementation in all kernels, including PREEMPT_RT kernels. Use raw_spinlock_t only in real critical core code, low-level interrupt handling and places where disabling preemption or interrupts is required, for example, to safely access hardware state. raw_spinlock_t can sometimes also be used when the critical section is tiny, thus avoiding RT-mutex overhead. How does it apply here? > Dave suggested to comment on the function rather than comment on the > raw_spin_lock directly, e.g., no other kernel code does that: > > https://lore.kernel.org/linux-mm/d2b3bc5e-1371-0c50-8ecb-64fc70917d42@xxxxxxxxx/ > > So I commented the function in this version: > > +/* > + * Do the module global initialization if not done yet. > + * It's always called with interrupts and preemption disabled. > + */ If interrupts are always disabled why do you need _irqsave()? -- Kiryl Shutsemau / Kirill A. Shutemov
