anchao commented on PR #15705: URL: https://github.com/apache/nuttx/pull/15705#issuecomment-2623616378
>No, it isn't truth. the critical section is a big lock which is a well known SMP isssue and should be replaced by the fine grant lock step by step: https://en.wikipedia.org/wiki/Giant_lock More and more IoT device has SMP capability, so it's important to improve NuttX's SMP performance. I agree with the changes to the critical section, but the current API changes are beyond my understanding and we are heading down the wrong path. >All known issues are identified and fixed timely(ALL within 24hours), do you have any new issue? You have too many mistakes in the entire change cycle. This is just the one point of my review. There are many more, so I won’t share too much. https://github.com/apache/nuttx/pull/15232#discussion_r1888289943 > Let's emphasize that the critical section isn't equal to a spin lock since the thread will leave the critical seciton if the thread is suspended by shceduler, but the spinlock doesn't have this capability that's why many places replace enter_critical_section with spin_lock_irqsave + sched_lock. > Where do you find we patch the sched_lock seperatedly? we change to spin_lock_irqsave+sched_lock at the same time. > No, #14578 was created three months ago to fix #9531 and the problem I mentioned before. But since it take a long time to review this important change, we have to use spin_lock_irqsave+sched_lock before it get merged. Let's refrain from unfounded remarks. You intended to replace wo_trace with the raw_ version to avoid the implementation of trace. I pointed out the issues in this approach within PR14943. Nevertheless, you were rather determined to merge the raw_ implementation. https://github.com/apache/nuttx/pull/14943#issuecomment-2606716195 > Yes, since this place is identified carefully that the lock scheduler sin't really needed. No, let us keep the semantics of spin_lock_irqsave and not extend sched_lock and not follow linux which is an old implementation > This is the wrong usage of spinlock which hold the lock in one thread and unlock in another thread. who wrong？It is very irresponsible of you to say this. There is nothing wrong with ESP using spin_lock_irqsave. It is your changes that caused the regression. Why do you say it is someone else's fault? It is unreasonable. > @hujun260 could we use a new approach to avoid touching esp32 code? stop give the new API, just keep original API semantics > please look at the dissusion: #9531 The naming of the API can show the implementation content, which is a perfect naming method. spin_lock_irqsave contains two functions: ``` 1. spin_lock 2. irqsave ``` Why do you have to make spin_lock_irqsave contain three functions? ``` 1. spin_lock 2. irqsave 3. sched_lock <--- why? ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
