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

cpufreq: dynamically alloc MAX_AVAIL_FREQS according to time_in_state #4108

Merged
merged 4 commits into from
Apr 23, 2023
Merged

cpufreq: dynamically alloc MAX_AVAIL_FREQS according to time_in_state #4108

merged 4 commits into from
Apr 23, 2023

Conversation

zzzyhtheonly
Copy link
Member

@zzzyhtheonly zzzyhtheonly commented Apr 20, 2023

Hi, there is an error report "cpufreq plugin: Found too many frequency states (21 > 20). Plugin needs to be recompiled. Please open a bug report for this." on our machines.

We are using Nvidia's ARMv8 processors and Linux kernel based on 4.14.150. We find that there are more than 20 in cpu's freq table:

cat /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state | wc -l
23

We try to resolve this by making MAX_AVAIL_FREQS a configurable variable in conf file in order to avoid recompiling. And it is tested on our machines (we make MAX_AVAIL_FREQS to 30 in our conf files).
Please help review if this idea works, if not, should we simply change MAX_AVAIL_FREQS into a bigger one?
Thanks a lot!

ChangeLog: cpufreq: make MAX_AVAIL_FREQS configurable in conf

Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
@zzzyhtheonly zzzyhtheonly requested a review from a team as a code owner April 20, 2023 09:15
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
@eero-t
Copy link
Contributor

eero-t commented Apr 20, 2023

Changing plugin to allocate the array dynamically is good idea, but this PR does not go far enough. Code should check how many CPUs there actually are, and allocate enough space for all of them instead of requiring user to determine that.

PS. Nowadays 20 cores is indeed a ridiculously low number, as there are servers with much larger core counts.

Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
@zzzyhtheonly
Copy link
Member Author

@eero-t
Thanks for your comment! MAX_AVAIL_FREQS states the total number of frequencies supported by
each CPU, referenced at https://www.kernel.org/doc/Documentation/cpu-freq/cpufreq-stats.txt
But yes, we should check how many frequencies there actually are instead of requiring users to determine that, so I patch this PR and please help review again. Thanks!

@mrunge
Copy link
Member

mrunge commented Apr 23, 2023

Thank you for the PR, this looks like a really good idea. I wouldn't mind to start with a larger number (e.g 128) from the beginning.

The CI failures are unfortunately known and not caused by this PR!

Signed-off-by: tiozhang <zyhtheonly@yeah.net>
@zzzyhtheonly zzzyhtheonly changed the title cpufreq: make MAX_AVAIL_FREQS configurable in conf cpufreq: dynamically alloc MAX_AVAIL_FREQS according to time_in_state Apr 23, 2023
@mrunge mrunge merged commit 5332f43 into collectd:main Apr 23, 2023
17 of 22 checks passed
@mrunge
Copy link
Member

mrunge commented Apr 23, 2023

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.

None yet

3 participants