Skip to content

Conversation

JakeHillion
Copy link
Contributor

@JakeHillion JakeHillion commented Dec 3, 2024

Layered previously hardcoded the defaults for slice_ns and max_exec_us. This
was changed in a recent PR to get these from CO:RE in the running kernel. This
change doesn't work because the old values are still set a few lines below.

Check for the presence of the CLI options and default to the CO:RE values
otherwise. Remove setting the layer struct's slice_ns as we check if it's equal
to 0 anyway and default to the global in the BPF code.

Test plan:

  • CI

@JakeHillion JakeHillion requested review from etsal and htejun December 3, 2024 18:50
@etsal
Copy link
Contributor

etsal commented Dec 3, 2024

I think ideally we should have --slice-us and --max-exec-us to not have a default and only override the system default if set. The current code definitely does not use the builtin values because there is both a default and a possibly user-defined value provided from the scheduler, and it makes sense for both to override the system default. I think it makes sense to removes the scheduler-provided defaults and use the system ones instead, but the current code AFAICT breaks the --slice-us and --max-exec-us switches.

@JakeHillion
Copy link
Contributor Author

I think ideally we should have --slice-us and --max-exec-us to not have a default and only override the system default if set. The current code definitely does not use the builtin values because there is both a default and a possibly user-defined value provided from the scheduler, and it makes sense for both to override the system default. I think it makes sense to removes the scheduler-provided defaults and use the system ones instead, but the current code AFAICT breaks the --slice-us and --max-exec-us switches.

Ah sure, you're right! Will update and test this properly. Spotted going through that the current behaviour (setting it twice) clearly wasn't what was intended and thought it was an easy fix.

Layered previously hardcoded the defaults for slice_ns and max_exec_us. This
was changed in a recent PR to get these from CO:RE in the running kernel. This
change doesn't work because the old values are still set a few lines below.

Check for the presence of the CLI options and default to the CO:RE values
otherwise. Remove setting the layer struct's slice_ns as we check if it's equal
to 0 anyway and default to the global in the BPF code.

Test plan:
- CI
@JakeHillion
Copy link
Contributor Author

Closing for now as it would need rebasing and isn't my priority at the moment. Would still be good to fix at some point in the future.

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