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_loader: Add scheduler loader via system DBUS interface #565

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

vnepogodin
Copy link
Contributor

@vnepogodin vnepogodin commented Aug 26, 2024

This pull request introduces a utility with a DBUS interface to initiate schedulers with any flags potentially supported by a specific scheduler. It accomplishes this by starting schedulers as child processes.

The scx_loader offers the following features:

  1. The StartScheduler method accepts the scx_name of the scheduler (e.g., "scx_rusty" or "scx_bpfland") and the scheduler mode (profile) as an enumerated value (raw input expects an unsigned integer).
  2. The StartSchedulerWithArgs method accepts the scx_name of the scheduler and the scheduler CLI arguments.
  3. The StopScheduler method can be used to terminate a running scheduler.
  4. The CurrentScheduler property indicates the currently active scheduler; if none is running, it returns "unknown." (Note: This property may be modified in the future to return an error instead.)
  5. The SchedulerMode property provides information about the currently set scheduler mode.
  6. The SupportedSchedulers property provides the list of currently supported schedulers.

vnepogodin added a commit to CachyOS/scx that referenced this pull request Aug 26, 2024
convert scheduler crate into lib+bin crate, to be able to
use(load/unload) scheduler by other crates

required for sched-ext#565
@htejun
Copy link
Contributor

htejun commented Aug 26, 2024

What are the benefits of turning each scheduler into library instead of just having a dbus attached launcher? The cost is that this bifurcates how schedulers are run depending on how it's run which does make things a bit more obscure. So, my preference would be layering on top.

@vnepogodin
Copy link
Contributor Author

What are the benefits of turning each scheduler into library instead of just having a dbus attached launcher? The cost is that this bifurcates how schedulers are run depending on how it's run which does make things a bit more obscure. So, my preference would be layering on top.

Generally loading schedulers as native code(crates), is more reliable and self-contained, allows for better integration with schedulers if needed. Although I will rework scx_loader to manage child processes instead of loading/unloading schedulers.

In future some schedulers can adjusted separately to run as crates instead of child proc if needed

scheds/rust/Cargo.toml Outdated Show resolved Hide resolved
env: cargo_env,
depends: sched_deps,
build_by_default: false,
build_always_stale: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you pull in the latest, scheds/rust/meson.build has an example of how to hook this up from the parent directory. I think we just need to add the same template to rust/meson.build, specify scx_loader as the only target (as others are libraries) and make the top level meson.build to descend into the rust directory.

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'm tried to add that. seems to work)

Although installation of dbus spec is a bit messy :)

@htejun
Copy link
Contributor

htejun commented Aug 28, 2024

Also, would it make sense to make the name a bit more specific - e.g. scx_dbus_loader?

@vnepogodin vnepogodin force-pushed the feature/scx-loader branch 6 times, most recently from ea4456f to 7c2822f Compare August 29, 2024 16:48
@vnepogodin vnepogodin marked this pull request as ready for review August 29, 2024 16:48
@vnepogodin
Copy link
Contributor Author

vnepogodin commented Aug 29, 2024

Also, would it make sense to make the name a bit more specific - e.g. scx_dbus_loader?

I think it would be a bit verbose, and if we would add some schedulers as crates would require rename again. I don't think we need both loaders at the same time in that case

@vnepogodin vnepogodin force-pushed the feature/scx-loader branch 4 times, most recently from d3164af to ff65cba Compare August 29, 2024 17:48

# Install bus spec for scx_loader
if [ "$name" = "scx_loader" ]; then
install -D -m 0644 "${source_dir}/org.scx.Loader.conf" "${DESTDIR}/usr/share/dbus-1/system.d/org.scx.Loader.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether the right thing to do is invoking cargo install with the right parameters. e.g. if someone does cargo install scx_loader, it should also install the dbus-1 file too, right?

Copy link
Contributor Author

@vnepogodin vnepogodin Aug 29, 2024

Choose a reason for hiding this comment

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

yeah it's required. hmm I don't think its possible to install custom files with cargo install. would be great)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can include custom files in the crate via include, see for example https://github.com/sched-ext/scx/blob/main/rust/scx_rustland_core/Cargo.toml#L26. But you can't arbitrarily install them anywhere in the system I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like not possible yet to install custom files with cargo install, but possible with tools like cargo xtask

rust-lang/cargo#2729

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Let's leave as-is for the time being.

env: cargo_env,
depends: sched_deps,
build_by_default: true,
build_always_stale: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it explicitly build only rust_binaries. No need to build libraries separately by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean to set build_always_stale to false on rust_binaries?

Copy link
Contributor

Choose a reason for hiding this comment

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

There need to be two targets:

  1. The default target which gets triggered by default.
  2. Package-specific target so that users can do meson compile -C build $TARGET.
    We don't want to build libraries by default for 1) but want to provide (we weren't before) individual targets for 2).

So, the default target should do cargo build -p scx_loader. It will probably look something like:

rust_libs = [ ... ]
rust_bins = [ ... ]
rust_all = rust_libs + rust_bins

// `-p bin` for each in rust_bin to cargo args
custom_target('rust_binaries', ...)

foreach package in rust_all
    custom_target(package, ...)

Please feel free to leave it as is. I can update afterwards too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I'll leave it as is then )

@vnepogodin
Copy link
Contributor Author

vnepogodin commented Aug 29, 2024

@htejun I'm currently thinking if we should provide additional systemd service for scx_loader to be able to autostart it by DBUS, even if the service was enabled/started by user, DBUS will search by name and start it with propagation of called dbus method/property

@htejun htejun merged commit 4513dfb into sched-ext:main Sep 4, 2024
1 check failed
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.

3 participants