Hi Chao, For these two options:
*Option 1:* spin_lock: spin lock spin_lock_nopreempt: spin_lock + sched_lock spin_lock_irqsave: spin lock + irqsave spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock *Option 2:* spin_lock: spin lock + sched_lock spin_lock_preempt: spin_lock spin_lock_irqsave: spin lock + irq save +sched_lock spin_lock_irqsave_preempt: spin_lock + irq save >From the correctness level: The Option2 is correct, because if you call sem_post()/syslog()/... within spin_lock() will cause a system crash. >From the performance level: The Option2 has lower efficiency as you said in the NON-SMP case. Then can we have a method that takes both ? Based on option1, we add a check if someone called sem_post()/syslog()... then system ASSERT. Alert the people who should change their usage. And also the performance will be considered. BRs, ligd chao an <magicd...@gmail.com> 于2025年2月5日周三 19:34写道： > I do not agree with the revert related changes. Local locks are very > helpful for system real-time performance. I just have some suggestions on > API semantics. > > Changes of kernel API semantics need to be carefully considered, especially > if these functions will be used by individual developers and projects. The > new semantics should be named with a new API to minimize the impact. > > BRs, > > hujun260 <hujun...@163.com> 于2025年2月5日周三 19:19写道： > > > I reverted the relevant changes. > > > > > > https://github.com/apache/nuttx/pull/15767 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > At 2025-02-05 13:06:29, "chao an" <magicd...@gmail.com> wrote: > > >Hi, > > > > > >The behavior of spin_lock needs everyone's advice > > > > > >After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged > > into > > >the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the > > >feature of disable the preemption: > > > > > >https://github.com/apache/nuttx/pull/14578 > > > > > >The spin lock behavior changed from: > > > > > > > > >*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave* > > > > > >to: > > > > > > > > >*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock + > irqsave + > > >sched_lock* > > > > > >This change has two key issues > > >1. *Crash*: Since spin_lock depends on sched_lock, the code using this > > type > > >of API needs to re-evaluate the scope of impact, especially when tcb is > > not > > >initialized at the startup stage, sched_lock will cause crash > > > > > >https://github.com/apache/nuttx/pull/15728 > > > > > >2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used > > in > > >the kernel, and more than 90% of the code protected by spin_lock will > not > > >cause context switching, so after the introduction of sched_lock, the > > >kernel part has introduced new performance overhead > > > > > >https://github.com/apache/nuttx/pull/15684 > > > > > > > > >Due to the change in API semantics, we need to further optimize all the > > >locations in the kernel that use spin_lock()/spin_lock_irqsave() to > solve > > >performance issue in evolution. > > >In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had > > some > > >arguments about API naming with me. > > > > > >https://github.com/apache/nuttx/pull/15705 > > > > > >My point of view is to restore the original behavior of spin_lock() and > > >spin_lock_irqsave(), which brings the following benefits: > > >1. The API semantics remain unchanged, and independent developers and > > >projects that use these APIs outside the nuttx repository can keep their > > >code without any adjustment > > >2. The API naming is consistent with the internal implementation, and > the > > >caller can know what is happening inside the function from the API > naming > > > > > >*Option 1:* > > > > > > > > > > > >*spin_lock: spin lockspin_lock_nopreempt: > > >spin_lock + sched_lockspin_lock_irqsave: spin lock + irq > > >savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock* > > > > > >@xiaoxiang suggested changing the semantics of the API to hold > sched_lock > > >by default, consistent with Linux: > > >1. This means that all locations in the kernel that call spin_lock and > > >spin_lock_irqsave need to be changed > > >spin_lock_irqsave -> spin_lock_irqsave_preempt > > >2. It is impossible to know what happens inside the function from the > API > > >naming > > > > > >*Option 2:* > > > > > > > > > > > >*spin_lock: spin lock + sched_lockspin_lock_preempt: > > > spin_lockspin_lock_irqsave: spin lock + irq save + > > >sched_lockspin_lock_irqsave_preempt: spin_lock + irq save* > > > > > >I don't know which definition is better, or if any wise person has a > > better > > >choice, please give some advice. > > >
