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

Added support for per CPU core monitoring #301

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Conversation

freysteinn
Copy link
Contributor

This commit adds multicore support to the stat_iterate.sh script and to the CpuStatsRunner. The previous script only read the first entry of the /proc/stat, which gave the average CPU usage for all cores. This modified script reads all cpu[0-9]+ records, including the average. It retains the output format of the original script; however, the implementation has some minor optimizations, which are not that significant due to how seldom the script runs.

  1. It primarily relies on the awk command for the heavy lifting, making it easier to read. Awk handles reading the /proc/stat instead of the cat command.

  2. The previous script used the seq command, which consumes more memory when multiple iterations occur. Awk handles this instead with a loop.

I tested the script using gawk with and without the Posix compliance flag (-P) and BusyBox v1.36.1 awk.

@freysteinn
Copy link
Contributor Author

I will have to update the unit-tests to account for the change.

@freysteinn
Copy link
Contributor Author

Do you have a recommended way of updating the test-rrul.flent.gz file to include per-CPU core records? Should I recreate the flent file? Or do you recommend doing this in another way?

@tohojo
Copy link
Owner

tohojo commented Apr 5, 2024

Do you have a recommended way of updating the test-rrul.flent.gz file to include per-CPU core records? Should I recreate the flent file? Or do you recommend doing this in another way?

test_plotters.py has a PLOTS_MAY_FAIL list for the plots that don't have data available in all test files. You can add the plots there; then keep the old data file around and add a new one that does contain the new data entries...

@freysteinn
Copy link
Contributor Author

freysteinn commented Apr 5, 2024

Do you have a recommended way of updating the test-rrul.flent.gz file to include per-CPU core records? Should I recreate the flent file? Or do you recommend doing this in another way?

test_plotters.py has a PLOTS_MAY_FAIL list for the plots that don't have data available in all test files. You can add the plots there; then keep the old data file around and add a new one that does contain the new data entries...

I've updated the pull request by adding the per-core CPU usage entries into the PLOTS_MAY_FAIL variable. However, I decided not to create a new flent file because the unit tests would miss any future broken entries while the PLOTS_MAY_FAIL is in effect.

I think it would be better to recreate the test-rrul.flent.gz or change the PLOTS_MAY_FAIL to include which files these plots may fail in. Then, if someone accidentally breaks it, they will fail in the future.

@tohojo
Copy link
Owner

tohojo commented Apr 5, 2024 via email

Some of the test Flent files lack data for specific plots. This lack of
data occurs either because the file did not include the parameter to
create them or because the feature did not exist when it was created.

The unit test framework ignores all plots that might lack data to ensure
they will not fail when unintentional changes break these tests.
However, this also means it will not catch changes that accidentally
break them.

This commit adds functionality to allow defining which plots should be
omitted by the unit tests on a per-file basis, allowing testing of files
that do include the plots.

Signed-off-by: Frey Alfredsson <[email protected]>
@freysteinn
Copy link
Contributor Author

I've updated this pull request to include a list of plots to ignore per unit-test file and added a test for the new per-core monitoring functionality.

This commit adds multicore support to the stat_iterate.sh script and to
the CpuStatsRunner. The previous script only read the first entry of the
/proc/stat, which gave the average CPU usage for all cores. This
modified script reads all cpu[0-9]+ records, including the average. It
retains the output format of the original script; however, the
implementation has some minor optimizations, which are not that
significant due to how seldom the script runs.

1. It primarily relies on the awk command for the heavy lifting, making
   it easier to read. Awk handles reading the /proc/stat instead of the
cat command.

2. The previous script used the seq command, which consumes more memory
   when multiple iterations occur. Awk handles this instead with a loop.

I tested the script using gawk with and without the Posix compliance
flag (-P), BusyBox v1.36.1 awk, and mawk 1.3.4.

Signed-off-by: Frey Alfredsson <[email protected]>
@freysteinn
Copy link
Contributor Author

I added that I tested the script with mawk 1.3.4 and added support for old versions of awk. However, I had no issues with the awk implementations I tested the script with.

Copy link
Owner

@tohojo tohojo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@tohojo tohojo merged commit 927f5b5 into tohojo:master Apr 9, 2024
58 checks passed
@freysteinn freysteinn deleted the ncpu branch April 11, 2024 00:17
@dtaht
Copy link
Collaborator

dtaht commented Jul 30, 2024

@freysteinn Wonderful thank you!

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