Skip to content
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.

Sign up for GitHub

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

Jump to bottom

scx_layered: Refactor proximity map iteration #1086

Merged
merged 1 commit into from
Dec 11, 2024
Merged

scx_layered: Refactor proximity map iteration #1086

merged 1 commit into from
Dec 11, 2024

Conversation

hodgesds
Copy link
Contributor

@hodgesds hodgesds commented Dec 9, 2024 •
edited
Loading

Refactor proximity map iteration order to radiate based on distance from current cpu. Example output from initialization:

22:56:44 [DEBUG] CPU[77] proximity map[2/40/40/80]: [77, 37, 78, 39, 76, 38, 79, 36, 75, 35, 74, 34, 73, 33, 72, 32, 71, 31, 70, 30, 69, 29, 68, 28, 67, 27, 66, 26, 65, 25, 64, 24, 63, 23, 62, 22, 61, 21, 60, 20, 59, 19, 58, 18, 57, 17, 56, 16, 55, 15, 54, 14, 53, 13, 52, 12, 51, 11, 50, 10, 49, 9, 48, 8, 47, 7, 46, 6, 45, 5, 44, 4, 43, 3, 42, 2, 41, 1, 40, 0]
22:56:44 [DEBUG] CPU[78] proximity map[2/40/40/80]: [78, 38, 79, 39, 77, 37, 76, 36, 75, 35, 74, 34, 73, 33, 72, 32, 71, 31, 70, 30, 69, 29, 68, 28, 67, 27, 66, 26, 65, 25, 64, 24, 63, 23, 62, 22, 61, 21, 60, 20, 59, 19, 58, 18, 57, 17, 56, 16, 55, 15, 54, 14, 53, 13, 52, 12, 51, 11, 50, 10, 49, 9, 48, 8, 47, 7, 46, 6, 45, 5, 44, 4, 43, 3, 42, 2, 41, 1, 40, 0]
22:56:44 [DEBUG] CPU[79] proximity map[2/40/40/80]: [79, 39, 78, 38, 77, 37, 76, 36, 75, 35, 74, 34, 73, 33, 72, 32, 71, 31, 70, 30, 69, 29, 68, 28, 67, 27, 66, 26, 65, 25, 64, 24, 63, 23, 62, 22, 61, 21, 60, 20, 59, 19, 58, 18, 57, 17, 56, 16, 55, 15, 54, 14, 53, 13, 52, 12, 51, 11, 50, 10, 49, 9, 48, 8, 47, 7, 46, 6, 45, 5, 44, 4, 43, 3, 42, 2, 41, 1, 40, 0]

@hodgesds hodgesds requested a review from htejun December 9, 2024 22:38
htejun
htejun reviewed Dec 9, 2024
View reviewed changes
scheds/rust/scx_layered/src/main.rs Outdated
}
vec.sort_by_key(|&num| {
let diff = if num < x { x - num } else { num - x };
(diff, num < x)
Copy link
Contributor

Choose a reason for hiding this comment

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

((num - x).abs(), num < x)?

scheds/rust/scx_layered/src/main.rs Outdated
});
let mut result = Vec::new();
let left: Vec<usize> = vec[0..vec.len() / 2].iter().cloned().rev().collect();
let right: Vec<usize> = vec[vec.len() / 2..vec.len()].iter().cloned().collect();
Copy link
Contributor

@htejun htejun Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If it's already sorted by abs distance, I'm not sure what the lef/right interposing achieves. The "center" the order is radiating form is already at the beginning of the list, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it's the difference between:

23:01:54 [DEBUG] CPU[79] proximity map[2/40/40/80]: [79, 39, 78, 38, 77, 37, 76, 36, 75, 35, 74, 34, 73, 33, 72, 32, 71, 31, 70, 30, 69, 29, 68, 28, 67, 27, 66, 26, 65, 25, 64, 24, 63, 23, 62, 22, 61, 21, 60, 20, 59, 19, 58, 18, 57, 17, 56, 16, 55, 15, 54, 14, 53, 13, 52, 12, 51, 11, 50, 10, 49, 9, 48, 8, 47, 7, 46, 6, 45, 5, 44, 4, 43, 3, 42, 2, 41, 1, 40, 0]

vs

23:03:01 [DEBUG] CPU[79] proximity map[2/40/40/80]: [79, 39, 78, 77, 76, 75, 74, 73, 72, 71, 70, 69, 68, 67, 66, 65, 64, 63, 62, 61, 60, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0]

@htejun
Copy link
Contributor

htejun commented Dec 9, 2024

One improvement may be sorting by the core_id distance and then using cpu_id distance as the sort sub-key. That way, the visiting order can be grouped by smt threads and that is a bit more orderly and might even have cache consequences.

@hodgesds hodgesds force-pushed the layered-prox-refactor branch from cf7ce2a to 2535e98 Compare December 9, 2024 22:57
@hodgesds
Copy link
Contributor Author

hodgesds commented Dec 9, 2024

One improvement may be sorting by the core_id distance and then using cpu_id distance as the sort sub-key. That way, the visiting order can be grouped by smt threads and that is a bit more orderly and might even have cache consequences.

I think that makes sense. Getting the order grouped by smt threads seems like the right approach, lemme rework this.

@hodgesds hodgesds force-pushed the layered-prox-refactor branch from 2535e98 to 378365f Compare December 10, 2024 00:13
@hodgesds
Copy link
Contributor Author

hodgesds commented Dec 10, 2024 •
edited
Loading

New ordering based on core id/cpu id:

04:49:31 [DEBUG] CPU[78] proximity map[2/40/40/80]: [78, 38, 79, 37, 39, 77, 36, 76, 35, 75, 34, 74, 33, 73, 32, 72, 31, 71, 30, 70, 29, 69, 28, 68, 27, 67, 26, 66, 25, 65, 24, 64, 23, 63, 22, 62, 21, 61, 20, 60, 9, 49, 8, 48, 7, 47, 6, 46, 5, 45, 4, 44, 3, 43, 2, 42, 19, 59, 18, 58, 17, 57, 16, 56, 15, 55, 14, 54, 13, 53, 12, 52, 11, 51, 10, 50, 1, 41, 0, 40]
04:49:31 [DEBUG] CPU[79] proximity map[2/40/40/80]: [79, 39, 38, 78, 37, 77, 36, 76, 35, 75, 34, 74, 33, 73, 32, 72, 31, 71, 30, 70, 29, 69, 28, 68, 27, 67, 26, 66, 25, 65, 24, 64, 23, 63, 22, 62, 21, 61, 20, 60, 9, 49, 8, 48, 7, 47, 6, 46, 5, 45, 4, 44, 3, 43, 2, 42, 19, 59, 18, 58, 17, 57, 16, 56, 15, 55, 14, 54, 13, 53, 12, 52, 11, 51, 10, 50, 1, 41, 0, 40]

@hodgesds hodgesds force-pushed the layered-prox-refactor branch 3 times, most recently from 83aeeef to 682ae6b Compare December 10, 2024 04:59
htejun
htejun reviewed Dec 11, 2024
View reviewed changes
scheds/rust/scx_layered/src/main.rs Outdated
vec.sort_by_key(|&id| {
(
(topo.all_cpus.get(&id).unwrap().core_id as i32 - center_core as i32).abs(),
center_cpu.cmp(&id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to also use abs distance from center_cpu here? Both sibling CPU IDs could be on the same side of the center and in such cases center_cpu.cmp(&id) wouldn't order them in a deterministic way. This likely doesn't matter in any practical sense but it does make the code confusing as it's unclear what the second ordering is supposed to do. The same for goes for radiate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, it was completely backwards. Should be good now.

@hodgesds hodgesds force-pushed the layered-prox-refactor branch from 682ae6b to 1bdf56a Compare December 11, 2024 14:23
@hodgesds
scx_layered: Refactor proximity map iteration
bbebb6e 
Refactor proximity map iteration order to radiate based on distance from
current cpu.

Signed-off-by: Daniel Hodges <hodges.daniel.scott@gmail.com>
@hodgesds hodgesds force-pushed the layered-prox-refactor branch from 1bdf56a to bbebb6e Compare December 11, 2024 15:23
@hodgesds hodgesds added this pull request to the merge queue Dec 11, 2024
Merged via the queue into sched-ext:main with commit c2e21ed Dec 11, 2024
23 checks passed
@hodgesds hodgesds deleted the layered-prox-refactor branch December 11, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htejun htejun htejun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hodgesds @htejun