Skip to content

Comments

feat: preemption between schedulers#266

Closed
kobayu858 wants to merge 31 commits intomainfrom
feat_sched_preempt
Closed

feat: preemption between schedulers#266
kobayu858 wants to merge 31 commits intomainfrom
feat_sched_preempt

Conversation

@kobayu858
Copy link
Contributor

@kobayu858 kobayu858 commented Jan 8, 2025

Implementation of inter-scheduler preemption for priority inversion.
Specifically, re-transmission of IPI, limitation of the period during which task status can be obtained, etc. were added.
Details are given below.

Test

kobayu858 added 12 commits December 27, 2024 17:33
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@kobayu858 kobayu858 requested a review from Koichi98 January 8, 2025 08:04
kobayu858 added 5 commits January 8, 2025 18:00
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@kobayu858 kobayu858 requested a review from atsushi421 January 8, 2025 11:10
kobayu858 added 3 commits January 8, 2025 21:00
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@kobayu858 kobayu858 requested a review from atsushi421 January 8, 2025 12:07
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@kobayu858 kobayu858 requested a review from Koichi98 January 8, 2025 12:46
kobayu858 added 3 commits January 8, 2025 22:00
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
fn invoke_preemption(&self, wake_task_priority: PriorityInfo) {
while let Some((task_id, cpu_id, lowest_priority_info)) = get_lowest_priority_task_info() {
if wake_task_priority < lowest_priority_info {
if IS_SEND_IPI.load(Ordering::Relaxed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have to use compare_exchange() here.
Checking and storing the value should be atomic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between executing get_lowest_priority_task_info and checking the IS_SEND_IPI value, is there a possibility that preemption could occur on another CPU, changing the lowest_priority task, and even completing the process of setting IS_SEND_IPI back to false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help me understand, could you answer the following question for both before and after merging this PR?

"Can a CPU receive and handle an IPI from another CPU while it's in invoke_preemption(), and then resume its original execution?"

Copy link
Contributor Author

@kobayu858 kobayu858 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that interrupts during lock would cause a deadlock or if invoke_preemption occurs during the task termination process, priority inversion may occur, so I changed the implementation.
Details are summarized in Doc and Doc

Copy link
Contributor Author

@kobayu858 kobayu858 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have to use compare_exchange() here.
Checking and storing the value should be atomic.

Correction. Thank you very much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between executing get_lowest_priority_task_info and checking the IS_SEND_IPI value, is there a possibility that preemption could occur on another CPU, changing the lowest_priority task, and even completing the process of setting IS_SEND_IPI back to false?

Yes. See Doc for how to deal with this after the implementation change.

Copy link
Contributor Author

@kobayu858 kobayu858 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help me understand, could you answer the following question for both before and after merging this PR?

"Can a CPU receive and handle an IPI from another CPU while it's in invoke_preemption(), and then resume its original execution?"

As mentioned earlier, interrupts during locking cause deadlocks, so no work properly after the merge.
Before merging, the GEDF data was locked, so no interruptions occurred during invoke_preemption(), and there were no issues.

kobayu858 added 2 commits January 14, 2025 19:12
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@kobayu858 kobayu858 requested a review from Koichi98 January 14, 2025 10:28
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Comment on lines 1178 to 1182
let mut is_send_ipi = IS_SEND_IPI.lock(&mut node);
if *is_send_ipi {
continue;
}
*is_send_ipi = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction. Thank you very much.

@atsushi421
Copy link
Contributor

@kobayu858
For readability, could you change the NOT_IN_TRANSITION to the IN_TRANSITION (affirmative form)?
If you rename them, you will also need to reverse their bool values.

let mut node = MCSNode::new();
let tasks = TASKS.lock(&mut node);
running_tasks
if true_count >= non_primary_cpus {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if true_count >= non_primary_cpus {
if true_count == non_primary_cpus {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction. Thank you very much.

.is_none_or(|(_, _, lowest_priority_info)| priority_info > *lowest_priority_info)
{
lowest_task = Some((task_id, cpu_id, priority_info));
if running_tasks.is_empty() || running_tasks.len() != non_primary_cpus {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if running_tasks.is_empty() || running_tasks.len() != non_primary_cpus {
if running_tasks.len() < non_primary_cpus {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running_tasks.is_empty() is required to pass make test.
Because at make tast, there is only one core, and running_tasks and non_primary_cpus seem to be set to 0.
Therefore, the above fix will cause an infinite loop during make test.

kobayu858 added 2 commits January 20, 2025 10:52
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
Signed-off-by: kobayu858 <yutaro.kobayashi@tier4.jp>
@ytakano ytakano closed this Feb 10, 2025
@kobayu858 kobayu858 deleted the feat_sched_preempt branch February 3, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants