feat: axtask set active cpu num #77
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces the ability to configure the number of active CPUs at runtime in the axtask module, rather than being limited to the compile-time constant axconfig::plat::CPU_NUM. The changes enable dynamic CPU configuration for task scheduling, which is particularly useful for scenarios where fewer CPUs should be active than the maximum supported by the platform.
Key Changes
- Added
init_scheduler_with_cpu_num()function to allow specifying active CPU count during initialization - Introduced runtime CPU count tracking via atomic
CPU_NUMvariable - Modified task CPU mask initialization to use active CPU count instead of the compile-time maximum
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| modules/axtask/src/api.rs | Adds CPU_NUM atomic variable, init_scheduler_with_cpu_num() function, and active_cpu_num() helper to support runtime CPU count configuration |
| modules/axtask/src/task.rs | Updates new_common() to initialize task CPU masks based on active CPU count instead of using AxCpuMask::full() |
| Cargo.lock | Dependency lock file updates, introduces duplicate axplat versions from different sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Initializes the task scheduler with cpu_num (for the primary CPU). | ||
| pub fn init_scheduler_with_cpu_num(cpu_num: usize) { | ||
| info!("Initialize scheduling..."); | ||
| CPU_NUM.store(cpu_num, core::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
The memory ordering used for storing and loading CPU_NUM should be Ordering::Release and Ordering::Acquire respectively (or stronger), not Ordering::Relaxed.
Currently, Relaxed ordering provides no synchronization guarantees. When init_scheduler_with_cpu_num stores the CPU count, tasks created immediately after in crate::run_queue::init() (line 84) call active_cpu_num() to read this value. Without proper ordering, there's no guarantee the write is visible to these reads, potentially causing tasks to be initialized with an incorrect CPU mask.
Recommended fix:
CPU_NUM.store(cpu_num, core::sync::atomic::Ordering::Release);and
CPU_NUM.load(core::sync::atomic::Ordering::Acquire)| } | ||
|
|
||
| pub(crate) fn active_cpu_num() -> usize { | ||
| CPU_NUM.load(core::sync::atomic::Ordering::Relaxed) |
There was a problem hiding this comment.
The memory ordering should be Ordering::Acquire to properly synchronize with the Release store in init_scheduler_with_cpu_num.
Using Relaxed ordering provides no guarantees that the value written by the primary CPU during initialization will be visible to secondary CPUs or to code reading this value later. This could lead to tasks being created with incorrect CPU masks.
Recommended fix:
CPU_NUM.load(core::sync::atomic::Ordering::Acquire)| CPU_NUM.load(core::sync::atomic::Ordering::Relaxed) | |
| CPU_NUM.load(core::sync::atomic::Ordering::Acquire) |
| init_scheduler_with_cpu_num(axconfig::plat::CPU_NUM); | ||
| } | ||
|
|
||
| /// Initializes the task scheduler with cpu_num (for the primary CPU). |
There was a problem hiding this comment.
The new public API function init_scheduler_with_cpu_num is missing documentation. Public APIs should have doc comments explaining their purpose, parameters, and any safety considerations.
Recommended addition:
/// Initializes the task scheduler with a specific number of active CPUs (for the primary CPU).
///
/// This allows limiting the number of active CPUs to fewer than the compile-time maximum
/// specified by `axconfig::plat::CPU_NUM`.
///
/// # Arguments
///
/// * `cpu_num` - The number of active CPUs to use. Must be <= `axconfig::plat::CPU_NUM`.
pub fn init_scheduler_with_cpu_num(cpu_num: usize) {| /// Initializes the task scheduler with cpu_num (for the primary CPU). | |
| /// Initializes the task scheduler with a specific number of active CPUs (for the primary CPU). | |
| /// | |
| /// This allows limiting the number of active CPUs to fewer than the compile-time maximum | |
| /// specified by `axconfig::plat::CPU_NUM`. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `cpu_num` - The number of active CPUs to use. Must be <= `axconfig::plat::CPU_NUM`. |
| } | ||
|
|
||
| /// Initializes the task scheduler with cpu_num (for the primary CPU). | ||
| pub fn init_scheduler_with_cpu_num(cpu_num: usize) { |
There was a problem hiding this comment.
Missing input validation for cpu_num. The function should validate that cpu_num is within valid bounds (1 <= cpu_num <= axconfig::plat::CPU_NUM).
If cpu_num is 0, tasks would be created with empty CPU masks, leading to a panic in select_run_queue_index (which asserts !cpumask.is_empty()). If cpu_num > axconfig::plat::CPU_NUM, the loop in new_common would attempt to set bits beyond the valid range of the AxCpuMask.
Recommended fix:
pub fn init_scheduler_with_cpu_num(cpu_num: usize) {
assert!(
cpu_num > 0 && cpu_num <= axconfig::plat::CPU_NUM,
"cpu_num must be in range [1, {}], got {}",
axconfig::plat::CPU_NUM,
cpu_num
);
info!("Initialize scheduling...");
CPU_NUM.store(cpu_num, core::sync::atomic::Ordering::Release);
// ...
}| pub fn init_scheduler_with_cpu_num(cpu_num: usize) { | |
| pub fn init_scheduler_with_cpu_num(cpu_num: usize) { | |
| assert!( | |
| cpu_num > 0 && cpu_num <= axconfig::plat::CPU_NUM, | |
| "cpu_num must be in range [1, {}], got {}", | |
| axconfig::plat::CPU_NUM, | |
| cpu_num | |
| ); |
| pub fn init_scheduler_with_cpu_num(cpu_num: usize) { | ||
| info!("Initialize scheduling..."); | ||
| CPU_NUM.store(cpu_num, core::sync::atomic::Ordering::Relaxed); | ||
| crate::run_queue::init(); | ||
| #[cfg(feature = "irq")] | ||
| crate::timers::init(); | ||
|
|
||
| info!(" use {} scheduler.", Scheduler::scheduler_name()); | ||
| } |
There was a problem hiding this comment.
The new public API function init_scheduler_with_cpu_num lacks test coverage. Given that the module has comprehensive tests in tests.rs for other scheduler functionality, this new function should also be tested.
Consider adding test cases that verify:
- Tasks created after calling
init_scheduler_with_cpu_numhave the correct CPU mask (matching the specified cpu_num) - The function properly handles edge cases (cpu_num = 1, cpu_num = max)
- Invalid inputs are properly rejected (if validation is added)
No description provided.