> 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.
Hi, Guiding I fully agree with your opinion. This is also the suggestion I talked with Xiaoxiang in WeChat before. We need to check whether the irq is saveed or the preemption is disabled in *spin_lock()*, and trigger the assertion if we find the wrong usage. BRs, Guiding Li <ligd...@gmail.com> 于2025年2月5日周三 19:57写道： > 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. > > > > > >
