sched: remove all spin_lock_irqsave(NULL) #15324
Conversation
github-actions
bot
added
Area: Drivers
|
(this comment is not specific to this PR) |
We replace a big lock with smaller locks. Generally, it's easy to find which lock protects what through file searching. |
sometimes it's obvious. sometimes not. |
| #ifdef CONFIG_RTC_HIRES | ||
| if (g_rtc_enabled) | ||
| { | ||
| up_rtc_gettime(&ts); |
There was a problem hiding this comment.
race condition may in lower half
| #ifdef CONFIG_RTC_HIRES | ||
| if (g_rtc_enabled) | ||
| { | ||
| up_rtc_gettime(&bias); |
| if (g_rtc_enabled) | ||
| { | ||
| irqstate_t flags; | ||
|
|
||
| up_rtc_gettime(ts); |
There was a problem hiding this comment.
g_basetime_lock is just use for protect g_basetime
There was a problem hiding this comment.
added c_section protection to up_rtc_gettime in driver code
There was a problem hiding this comment.
Removing the global lock is to improve performance, but adding c_section will make the performance worse, right?
There was a problem hiding this comment.
This critical section is temporarily added. To avoid potential issues, it will be gradually removed entirely in the future. The goal is to replace the critical section with a spinlock.
There was a problem hiding this comment.
So what's the point of removing the global lock? Make the code more complicated and make the performance worse?We are not robots. If it is just a simple replacement of the lock, why not let AI do it? Any changes need to think about how to make the code better.
There was a problem hiding this comment.
I just only one opinion "We can't let the performance get worse after any change" right?
spin_lock_irqsave(NULL) ->
spin_lock_irqsave(lock) + enter_critical_section()
up_rtc_gettime() is called very frequently. If the timer driver needs enter_critical_section() protection, it means that the spinlock global lock is not really removed yet.
There was a problem hiding this comment.
The entire replacement process needs to be carried out step by step. We can quickly submit a patch to replace the critical sections in the up_rtc_gettime driver as well.
There was a problem hiding this comment.
Please
- Resolve the lower half resource exclusive problem first
- Then delete the global spinlock
There was a problem hiding this comment.
I added spinlock to protect up_rtc_gettime
github-actions
bot
added
Arch: arm
hujun260
force-pushed
the
apache_53
branch
3 times, most recently
from
00ee1d0 to
68b83f3
Compare
Signed-off-by: hujun5 <[email protected]>
Signed-off-by: hujun5 <[email protected]>
…sched dir Signed-off-by: hujun5 <[email protected]>
reason: We have removed the critical section protection for the up_rtc_gettime function in common code. Signed-off-by: hujun5 <[email protected]>
Summary
sched: remove all spin_lock_irqsave(NULL)
Impact
sched
Testing
ci
qemu-armv8a:nsh_smp