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_stats: Drop sched-ext namespace #573

Merged
merged 3 commits into from
Sep 2, 2024
Merged

Conversation

sirlucjan
Copy link
Contributor

Since scx_stats has been implemented, namespace is already redundant.

Signed-off-by: Piotr Gorski <lucjan.lucjanov@gmail.com>
Copy link
Contributor

@arighi arighi left a comment

Choose a reason for hiding this comment

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

Left a couple of comments (feel free to ignore), but overall looks good!


```
journalctl --namespace=sched-ext --disk-usage
scx_SCHEDNAME --monitor X
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to --monitor SEC here to make it more clear that the argument defines the interval (in seconds) to print the output.

```

X - for example 1 - this will print the output every second


Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe add a real example, like:

scx_bpfland --monitor 0.5

To make it even more clear and to show that it also supports numbers with decimals.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree adding real examples would be nicer here.
For example, in case of LAVD, its monitoring option is a bit different from the others.

/// Run in monitoring mode. Show the specified number of scheduling
/// samples every second.
#[clap(long)]
monitor_sched_samples: Option<u64>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you take a look, please?

@sirlucjan
Copy link
Contributor Author

@arighi I don't like to ignore wise advice. Is it better now?

@multics69 multics69 self-requested a review August 29, 2024 12:20
@sirlucjan
Copy link
Contributor Author

@arighi @multics69

Maybe we should move the log information to a separate README or write information about it in the main one?

@arighi
Copy link
Contributor

arighi commented Aug 29, 2024

Hm... maybe the monitor info should go in the main README, since they're affecting everyone and not only those that are running the schedulers via systemd.

@sirlucjan
Copy link
Contributor Author

Let me know if the corrected examples are good and I will send the revisions to README.

Signed-off-by: Piotr Gorski <lucjan.lucjanov@gmail.com>
README.md Outdated
- scx_rustland

```
❯ run0 scx_rustland --monitor 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using here run0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

for example 0.5 - this will print the output every half a second

```
scx_bpfland --monitor 0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally would remove this part with the "for example 0.5". This just makes the README bigger with not much value

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 think such an example can stay to show exactly what it is about, but if more people think the same, I will remove it.

README.md Outdated
kick_greedy_cpus=f
NODE[00] load= 0.17 imbal= +0.00 delta= +0.00
DOM[00] load= 0.17 imbal= +0.00 delta= +0.00
###### Thu, 29 Aug 2024 14:42:42 +0200, load balance @ -1268.9ms ######
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also reduce here the logs, so that the README is not too much filled with logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@ptr1337 ptr1337 left a comment

Choose a reason for hiding this comment

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

Do we really need to explain from every supported scx scheduler how to use monitor?

I think you could write, which are the supported schedulers and bring one example. instead of showing of every scheduler a example.

@@ -344,6 +344,87 @@ $ cargo install scx_rusty

and `scx_rusty` will be built and installed as `~/.cargo/bin/scx_rusty`.

## Checking scx_stats

- To check the scheduler statistics, use the
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like:

Most schedulers implement --monitor $INTERVAL which fetches and shows the detailed statistics from the running scheduler. Note that this does not launch the scheduler. The monitoring instance only shows the statistics. Some schedulers may implement different or multiple monitoring options. Refer to --help of each scheduler for details. Most schedulers also accept --stats $INTERVAL to print the statistics directly from the scheduling instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, could you take a look now @htejun

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use "statistics" consistently? Schedulers do print out logs like info and warning messages. It just doesn't print out detailed statistics by default.

Copy link
Contributor Author

@sirlucjan sirlucjan Aug 30, 2024

Choose a reason for hiding this comment

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

Done @htejun

@sirlucjan sirlucjan marked this pull request as ready for review August 30, 2024 17:32
@sirlucjan sirlucjan force-pushed the drop-journald branch 2 times, most recently from 5158d14 to ef28c19 Compare August 30, 2024 17:35
@sirlucjan
Copy link
Contributor Author

sirlucjan commented Aug 30, 2024

@ptr1337

Do we really need to explain from every supported scx scheduler how to use monitor?

I think you could write, which are the supported schedulers and bring one example. instead of showing of every scheduler a example.

@multics69 asked about examples.

Signed-off-by: Piotr Gorski <lucjan.lucjanov@gmail.com>
@arighi
Copy link
Contributor

arighi commented Sep 2, 2024

It looks like we have enough approvals and with scx_rustland also converted to use scx_stats I think we can safely apply this change.

@arighi arighi merged commit 4fd1ae0 into sched-ext:main Sep 2, 2024
1 check passed
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.

5 participants