Re: [PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[Yu Kuai <yukuai@kernel.org>]

Hi,

在 2025/8/18 19:54, Yunseong Kim 写道:
> Hi Yu,
>
> On 8/18/25 9:56 AM, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/08/17 19:31, Yunseong Kim 写道:
>>> The function ppl_io_unit_finished() uses a local_irq_save()/spin_lock()
>>> sequence. On a PREEMPT_RT enabled kernel, spin_lock() can sleep. Calling it
>>> with interrupts disabled creates an atomic context where sleeping is
>>> forbidden.
>>>
>> What? I believe spin_lock can never sleep.
> I think you might have been a bit surprised by me sending a patch out of
> the blue. It would be helpful to refer to the references below:
>
>   On PREEMPT_RT kernels, these lock types are converted to sleeping locks:
>    local_lock
>    spinlock_t
>    rwlock_t
>
> Link: https://docs.kernel.org/locking/locktypes.html#sleeping-locks
>
>>> Ensuring that the interrupt state is managed atomically with the lock
>>> itself. The change is applied to both the 'log->io_list_lock' and
>>> 'ppl_conf->no_mem_stripes_lock' critical sections within the function.
>>>
>>> Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
>>> ---
>>>    drivers/md/raid5-ppl.c | 12 ++++--------
>>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
>>> index 56b234683ee6..650bd59ead72 100644
>>> --- a/drivers/md/raid5-ppl.c
>>> +++ b/drivers/md/raid5-ppl.c
>>> @@ -553,15 +553,13 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
>>>          pr_debug("%s: seq: %llu\n", __func__, io->seq);
>>>    -    local_irq_save(flags);
>>> -
>>> -    spin_lock(&log->io_list_lock);
>>> +    spin_lock_irqsave(&log->io_list_lock, flags);
>   The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
>   have a few implications. For example, on a non-PREEMPT_RT kernel the
>   following code sequence works as expected:
>
>   local_irq_disable();
>   spin_lock(&lock);
>   
>   and is fully equivalent to:
>   
>   spin_lock_irq(&lock);
>   
>   Same applies to rwlock_t and the _irqsave() suffix variants.
>
> Link: https://docs.kernel.org/locking/locktypes.html#spinlock-t-and-rwlock-t

Yes, lessons are learned. Perhaps add a link tag in the commit message
just in case someone else will be confused?

Thanks,
Kuai

>
>>>        list_del(&io->log_sibling);
>>> -    spin_unlock(&log->io_list_lock);
>>> +    spin_unlock_irqrestore(&log->io_list_lock, flags);
>>>          mempool_free(io, &ppl_conf->io_pool);
>>>    -    spin_lock(&ppl_conf->no_mem_stripes_lock);
>>> +    spin_lock_irqsave(&ppl_conf->no_mem_stripes_lock, flags);
>> Please notice, local_irq_save + spin_lock is the same as
>> spin_lock_irqsave, I don't think your changes have any functonal
>> chagnes.
> This issue has also been a problem in other subsystems, such as USB:
>
> [BUG] usb: gadget: dummy_hcd: Sleeping function called from invalid
> context in dummy_dequeue on PREEMPT_RT
>
> Link: https://lore.kernel.org/lkml/20250816065933.EPwBJ0Sd@linutronix.de/t/#u
>
> I am currently contributing to the kernel to address the areas that need to
> adapt to this paradigm shift so that the PREEMPT_RT Linux kernel can be
> well supported. I have CC’d other RT people so they can also review
> this part.
>
>> Thanks,
>> Kuai
> Thank you!
>
> Yunseong
>