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

On OpenBSD, use the actual number of online CPUs for facts #65370

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

jasperla
Copy link
Contributor

@jasperla jasperla commented Nov 29, 2019

SUMMARY

This switches to use hw.ncpuonline instead of hw.ncpu when
showing the number of cpus/cores in the system

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

setup module

ADDITIONAL INFORMATION

In OpenBSD it's possible to disable SMT since release 6.4 as such the number of detected physical CPUs/cores with hw.ncpu may not actually be the number of CPUs/cores which are in use. The hw.ncpuonline sysctl value does correctly reflect that.

I think it makes sense to switch to using this value instead of hw.ncpu.

This switches to use hw.ncpuonline instead of hw.ncpu when
showing the number of cpus/cores in the system
@ansibot
Copy link
Contributor

ansibot commented Nov 29, 2019

@ansibot ansibot added affects_2.10 bsd bug core_review needs_triage owner_pr support:core needs_revision and removed core_review labels Nov 29, 2019
@ansibot ansibot added the stale_ci label Dec 7, 2019
@jillr jillr removed the needs_triage label Dec 12, 2019
@jillr jillr requested a review from samdoran Dec 12, 2019
Copy link
Contributor

@samdoran samdoran left a comment

This seems like a reasonable change. I tested on OpenBSD 6.6 and it works.

Please create a changelog fragment. See this fragment as an example.

We do not have any unit tests for openbsd.by currently. Would you be able to write unit tests to at cover this method?

@samdoran
Copy link
Contributor

samdoran commented Dec 13, 2019

I am not a BSD user, so one thing I'm wondering is if changing this will result in any surprises or if it will be ok since it's correcting an inaccurate count. Should we leave the the current processor_cores and processor_count untouched and add new fields such as enabled_cores and enabled_count, or does it just make sense to change this to only reflect the enabled count?

@grayed
Copy link

grayed commented Dec 13, 2019

@samdoran, I'm not a Linux user (no kidding). It looks like the process_cores is filled from # processors line in /proc/cpuinfo. Does the # processors change upon disabling hyperthreading? If yes then (I suppose) on OpenBSD it should change, too; if not then not. Same logic applies for processor_count.

@samdoran
Copy link
Contributor

samdoran commented Dec 16, 2019

@grayed On Linux, disabling hyperthreading does not reduce the physical core count but does reduce the number of CPUs

Comparison of lscpu

with-ht.txt
CPU(s):                40
Thread(s) per core:    2
Core(s) per socket:    10

without-ht.txt
CPU(s):                20
Thread(s) per core:    1
Core(s) per socket:    10

Comparison of facts

with-ht-facts.txt
        "ansible_processor_cores": 10,
        "ansible_processor_count": 2,
        "ansible_processor_vcpus": 40

without-ht-facts.txt
        "ansible_processor_cores": 10,
        "ansible_processor_count": 2,
        "ansible_processor_vcpus": 20

@ansibot ansibot removed the stale_ci label Dec 24, 2019
@ansibot ansibot added the stale_ci label Jan 1, 2020
@ansibot ansibot added core_review and removed needs_revision labels Mar 5, 2020
@ansibot ansibot added needs_revision and removed core_review labels Mar 29, 2020
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 5, 2020
@ansibot ansibot removed the owner_pr label Mar 4, 2021
@ansibot ansibot added the needs_rebase label Apr 6, 2021
@bcoca
Copy link
Member

bcoca commented May 4, 2022

waiting_on_contributor

@ansibot ansibot added the waiting_on_contributor label May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 bsd bug needs_rebase needs_revision pre_azp support:core waiting_on_contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants