-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add rt_scheduler_critical_switch_flag to reduce scheduling on UP and fix bug on rzn2l #10581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📌 Code Review Assignment🏷️ Tag: bsp_renesasReviewers: kurisaW Changed Files (Click to expand)
🏷️ Tag: kernelReviewers: GorrayLi ReviewSun hamburger-os lianux-mm wdfk-prog xu18838022837 Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-08-06 15:24 CST)
📝 Review Instructions
|
src/scheduler_up.c
Outdated
| rt_hw_interrupt_enable(level); | ||
|
|
||
| if (rt_current_thread) | ||
| if (rt_scheduler_critical_switch_flag == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rt_schedule里面开始就会进行是否锁调度器的判断,没看出来这么修改后如何提升性能的?
麻烦举例说明下,谢谢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
更新:这个位置的修改是为了减少调度的次数吧?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是的,针对重复的调度进入,这是改动最小的方法吧,更改前后的对比可以看描述中的图(-O0时的数据)
麻烦签署下CLA,PR的标题也修改下吧,可以说明清晰点解决了什么问题 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a critical switch flag mechanism to optimize scheduler performance on single-core systems and fixes multiple issues in the rzn2l BSP, including duplicate interrupt handling and timer driver problems.
- Introduces
rt_scheduler_critical_switch_flagto track deferred scheduling needs and optimize performance on UP systems - Removes duplicate
rt_interrupt_enter/leavecalls in rzn2l timer callbacks that were causing issues - Fixes timer configuration and adds volatile keyword to prevent optimization issues
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scheduler_up.c | Adds critical switch flag mechanism to defer scheduling until safe to execute |
| src/cpu_up.c | Updates documentation to reflect deferred scheduling behavior |
| bsp/renesas/rzn2l_etherkit/rzn/fsp/src/bsp/cmsis/Device/RENESAS/Source/system.c | Adds volatile keyword to counter variables to prevent optimization issues |
| bsp/renesas/libraries/HAL_Drivers/drv_hwtimer.c | Removes duplicate interrupt handling for rzn2l and adds missing prescaler values |
| bsp/renesas/libraries/HAL_Drivers/config/rzt/timer_config.h | Increases prescaler array size to accommodate new values |
Comments suppressed due to low confidence (2)
src/scheduler_up.c:53
- [nitpick] The variable name
rt_scheduler_critical_switch_flagis overly verbose. Consider a shorter, clearer name likert_sched_deferred_flagorrt_critical_sched_flag.
static rt_int8_t rt_scheduler_critical_switch_flag = -1;
bsp/renesas/libraries/HAL_Drivers/drv_hwtimer.c:22
- The array size increase from 8 to 9 in timer_config.h doesn't match the addition of only 2 new prescaler values (lines 41-42). This could lead to array bounds issues or uninitialized elements.
#ifdef BSP_USING_TIM0
src/scheduler_up.c
Outdated
|
|
||
| extern volatile rt_atomic_t rt_interrupt_nest; | ||
| static rt_int16_t rt_scheduler_lock_nest; | ||
| static rt_int8_t rt_scheduler_critical_switch_flag = -1; |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number -1 for initialization is unclear. Consider using a named constant or enum to make the different states more explicit (e.g., SCHED_FLAG_UNINITIALIZED = -1, SCHED_FLAG_IDLE = 0, SCHED_FLAG_PENDING = 1).
| static rt_int8_t rt_scheduler_critical_switch_flag = -1; | |
| static rt_int8_t rt_scheduler_critical_switch_flag = SCHED_FLAG_UNINITIALIZED; |
src/scheduler_up.c
Outdated
| rt_hw_interrupt_enable(level); | ||
|
|
||
| if (rt_current_thread) | ||
| if (rt_scheduler_critical_switch_flag == 1) |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 1 should be replaced with a named constant to improve code clarity and maintainability.
| if (rt_scheduler_critical_switch_flag == 1) | |
| if (rt_scheduler_critical_switch_flag == RT_SCHEDULER_CRITICAL_SWITCH_PENDING) |
|
AI审查真棒啊,那需要这样改吗,MP那里也是单纯的使用数字,两边同步个宏? 我想的是减少影响范围和差异,因此也直接使用数字 |
感觉宏好一些 |
|
请修改数字标志位为,方便管理、裁剪。 |
|
麻烦将commit修改为1个 |
|
更新:
|
| #ifdef BSP_USING_TIM0 | ||
| void timer0_callback(timer_callback_args_t *p_args) | ||
| { | ||
| #if !defined(SOC_SERIES_R9A07G0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我觉得如果你想在timer中断回调里去掉RTT的中断嵌套计数,以此来提升程序的性能,在自己的项目中应用就好了,但是这部分代码就不要提到主线了,而且你说的“重复调用rt_interrupt_enter以及rt_interrupt_leave”,没理解你意思
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是因为在libc里面会隐式调用一次:
rt-thread/libcpu/arm/cortex-r52/context_gcc.S
Line 149 in 236e830
| BL rt_interrupt_enter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那是不是建议直接修改:
rt-thread/libcpu/arm/cortex-r52/context_gcc.S
那上下文切换汇编里就不考虑此操作了,与其他libcpu统一一下规范,这么看也挺别扭的
src/scheduler_up.c
Outdated
|
|
||
| extern volatile rt_atomic_t rt_interrupt_nest; | ||
| static rt_int16_t rt_scheduler_lock_nest; | ||
| static rt_int8_t rt_scheduler_critical_switch_flag = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这块推荐这种写法,尽量不要使用魔法数字,语义不直观:
Line 104 in 32e93ad
| #define IS_CRITICAL_SWITCH_PEND(pcpu, curthr) (RT_SCHED_CTX(curthr).critical_switch_flag) |
| #ifdef BSP_USING_TIM0 | ||
| void timer0_callback(timer_callback_args_t *p_args) | ||
| { | ||
| #if !defined(SOC_SERIES_R9A07G0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是因为在libc里面会隐式调用一次:
rt-thread/libcpu/arm/cortex-r52/context_gcc.S
Line 149 in 236e830
| BL rt_interrupt_enter |
多个commit也没啥问题,只要不冲突&每个commit都说明提交的内容即可 |
感觉问题不大,rv里面也是这么干的,rt_interrupt_enter 作用是为了让内核判断是否在中断中。 |
|
那我归总一下吧。当前pr的关键点:
这两个明显影响使用,至于rt_interrupt_enter以及rt_interrupt_leave重复不在这个pr或者主线上更改。 我一会再调整一下:
|
|
这个修改直接导致我线程切换异常了,我恢复成5.2.1版本后,线程切换才变正常 |
|
除了这里跟5.2.1版本保持一致,其它代码跟5.2.2版本一模一样,线程切换就正常了 |
|
如果使用5.2.2版本,工程跑起来会有很多应用线程跑不起来 |
主线暂时没发现这个问题呢,可以给下复现代码 |
|
应用代码不好给出,我用的是GD32H757 |
|
同样的应用,5.2.2跑起来会经常发生部分应用线程跑不起来(10次有3-4次吧),然后我是一部分代码一部分代码屏蔽,测试,整整搞了一周,才发现是这里导致的 |
|
在应用线程初始化阶段,有时候会发生某些线程启动失败且某些线程没有初始化的现象,应该是线程切换出现了问题 |
解决了吗?看这句话的现象,如果初始化都没好,那肯定无法完美启动,更不用说调度了。 |
拉取/合并请求描述:(PR description)
[
为什么提交这份PR (why to submit this PR)
rt_interrupt_enter以及rt_interrupt_leave你的解决方案是什么 (what is your solution)
rt_scheduler_critical_switch_flag,使单核和多核策略同步更改前
更改后
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up