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.

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

scx_rustland: mitigate sub-optimal performance with offline CPUs #189

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

arighi
Copy link
Collaborator

@arighi arighi commented Mar 14, 2024

Most of the schedulers assume that the amount of possible CPUs in the system represents the actual number of CPUs available.

This is not always true: some CPUs may be offline or certain CPU models (AMD CPUs for example) may include unavailable CPUs in this number.

This can lead to sub-optimal performance or even errors in the scheduler (see for example [1][2]).

Ideally, we need to attack this issue in a more generic way, such as having a proper API provided by a C library, that can be used by all schedulers and the topology Rust module (scx_utils crate).

But for now, let's try to mitigate most of the common sub-optimal cases separately inside each scheduler.

For rustland we can apply some mitigations both in select_cpu() (for the BPF part) and in the user-space part:

  • the former is fixed in the sched-ext kernel by commit 94dc0c01b957 ("scx: Use cpu_online_mask when resetting idle masks"). However, adding an extra check cpu < num_possible_cpus in select_cpu(), allows to properly support AMD CPUs, even with kernels that don't have the cpu_online_mask fix yet (this doesn't always guarantee the validity of cpu, but it should be enough to mitigate the majority of the potential sub-optimal cases, without introducing any significant overhead)

  • the latter can be fixed relying on topology.span(), instead of topology.nr_cpus(), to count the amount of available CPUs in the system.

[1] sched-ext/sched_ext#69
[2] #147

Link: sched-ext/sched_ext@94dc0c0

Most of the schedulers assume that the amount of possible CPUs in the
system represents the actual number of CPUs available.

This is not always true: some CPUs may be offline or certain CPU models
(AMD CPUs for example) may include unavailable CPUs in this number.

This can lead to sub-optimal performance or even errors in the scheduler
(see for example [1][2]).

Ideally, we need to attack this issue in a more generic way, such as
having a proper API provided by a C library, that can be used by all
schedulers and the topology Rust module (scx_utils crate).

But for now, let's try to mitigate most of the common sub-optimal cases
separately inside each scheduler.

For rustland we can apply some mitigations both in select_cpu() (for the
BPF part) and in the user-space part:

 - the former is fixed in the sched-ext kernel by commit 94dc0c01b957
   ("scx: Use cpu_online_mask when resetting idle masks"). However,
   adding an extra check `cpu < num_possible_cpus` in select_cpu(),
   allows to properly support AMD CPUs, even with kernels that don't
   have the cpu_online_mask fix yet (this doesn't always guarantee the
   validity of cpu, but it should be enough to mitigate the majority of
   the potential sub-optimal cases, without introducing any significant
   overhead)

 - the latter can be fixed relying on topology.span(), instead of
   topology.nr_cpus(), to count the amount of available CPUs in the
   system.

[1] sched-ext/sched_ext#69
[2] #147

Link: sched-ext/sched_ext@94dc0c0
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
@@ -264,14 +264,15 @@ impl<'a> Scheduler<'a> {
let init_page_faults: u64 = 0;

// Low-level BPF connector.
let nr_online_cpus = topo.span().weight() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, there's a small bug in topo where it computes span, that I'm about to send a fix for. Once that's addressed, you shouldn't need to do + 1 here. I'll merge this and update your code here as part of that fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, there's a small bug in topo where it computes span, that I'm about to send a fix for. Once that's addressed, you shouldn't need to do + 1 here. I'll merge this and update your code here as part of that fix.

I actually seemed like a bug, but I wasn't sure, thanks for clarifying it!

Copy link
Contributor

Choose a reason for hiding this comment

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

#191 should fix it

@Byte-Lab Byte-Lab merged commit 0ecac96 into main Mar 14, 2024
1 check passed
@Byte-Lab Byte-Lab deleted the rustland-offline-cpus branch March 14, 2024 16:02
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.

None yet

2 participants