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

Ensure max_threads is set for all CPUs defined under /hardware/cpu/ #161

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

maxeem
Copy link
Contributor

@maxeem maxeem commented Aug 14, 2024

This was needed to avoid hitting a bug when using the get_num_of_max_threads() function.

This assumes that hyperthreading is enabled.

For CPUs that do not support hyperthreading it sets max_threads to the value of cores count.

@jrha
Copy link
Member

jrha commented Aug 22, 2024

Why not fix get_num_of_cores_max_threads to handle max_threads being undefined?

@maxeem
Copy link
Contributor Author

maxeem commented Aug 22, 2024

In addition to templates changes? Or keeping old templates as they are?

I can make it default to cores count when needed, but some people might end up having less thread count by default.. which is not too bad but probably not optimal 🤷‍♂️

@jrha
Copy link
Member

jrha commented Aug 23, 2024

Yep, in addition to this change (which is fine in itself), but the schema doesn't enforce the property being present, so get_num_of_cores_max_threads shouldn't rely on it.

@jrha jrha added this to the 24.next milestone Sep 4, 2024
@jrha jrha force-pushed the ma_add_max_threads_to_cpu_definitions branch from 268d4f4 to ff9ee65 Compare September 5, 2024 10:33
This was needed to avoid hitting a bug when using the
get_num_of_max_threads() function.

This assumes that hyperthreading is enabled.

For CPUs that do not support hyperthreading it sets max_threads
to the value of cores count.
@jrha
Copy link
Member

jrha commented Sep 5, 2024

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants