-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
@@ -685,9 +694,10 @@ void rt_exit_critical(void) | |||
/* enable interrupt */ | |||
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_flag
to track deferred scheduling needs and optimize performance on UP systems - Removes duplicate
rt_interrupt_enter/leave
calls 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_flag
is overly verbose. Consider a shorter, clearer name likert_sched_deferred_flag
orrt_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
@@ -49,6 +50,7 @@ rt_uint8_t rt_thread_ready_table[32]; | |||
|
|||
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.
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; |
Copilot uses AI. Check for mistakes.
src/scheduler_up.c
Outdated
@@ -685,9 +694,10 @@ void rt_exit_critical(void) | |||
/* enable interrupt */ | |||
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.
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) |
Copilot uses AI. Check for mistakes.
AI审查真棒啊,那需要这样改吗,MP那里也是单纯的使用数字,两边同步个宏? 我想的是减少影响范围和差异,因此也直接使用数字 |
感觉宏好一些 |
请修改数字标志位为,方便管理、裁剪。 |
麻烦将commit修改为1个 |
更新:
|
@@ -184,8 +186,10 @@ static void timer_one_shot_check(void) | |||
#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
@@ -49,6 +50,7 @@ rt_uint8_t rt_thread_ready_table[32]; | |||
|
|||
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) |
@@ -184,8 +186,10 @@ static void timer_one_shot_check(void) | |||
#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或者主线上更改。 我一会再调整一下:
|
拉取/合并请求描述:(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