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.

Sign up for GitHub

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

Jump to bottom

Use online CPUs rather than max_ncpus for taskq thread count #10282

Merged
merged 1 commit into from
May 20, 2020
Merged

Use online CPUs rather than max_ncpus for taskq thread count #10282

merged 1 commit into from
May 20, 2020

Conversation

DeHackEd
Copy link
Contributor

@DeHackEd DeHackEd commented May 1, 2020

Some systems have issues or allow CPU hotplugging that can make
max_cpus absurdly large. As these systems are rare and the
consequences of too many threads can be debilitating limit this
to a known good value.

Signed-off-by: DHE git@dehacked.net

Motivation and Context

I have a machine where, for some reason, num_possible_cpus() returns 440. The correct number of CPUs/cores/threads in this system is 32. Don't know if that's a BIOS bug or something else, but having 440 instances of arc_prune "working" is a disaster.

I don't know if there's a better fix, but for now I'm drawing attention to what kills my system.

Alternative workaround: kernel commandline option nr_cpus=32 for this system.

Description

Change max_cpus to be the same as boot_cpus

How Has This Been Tested?

Build tested as a trivial change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 1, 2020
@codecov-io
Copy link

codecov-io commented May 2, 2020 •
edited
Loading

Codecov Report

Merging #10282 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10282      +/-   ##
==========================================
+ Coverage   79.52%   79.55%   +0.03%     
==========================================
  Files         389      389              
  Lines      123120   123120              
==========================================
+ Hits        97906    97945      +39     
+ Misses      25214    25175      -39
Flag Coverage Δ
#kernel 79.87% <ø> (-0.05%) ⬇️
#user 65.98% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
module/os/linux/spl/spl-kmem-cache.c 75.22% <0.00%> (-8.50%) ⬇️
cmd/zvol_id/zvol_id_main.c 76.31% <0.00%> (-5.27%) ⬇️
module/zfs/space_map.c 93.06% <0.00%> (-5.20%) ⬇️
module/zfs/spa_checkpoint.c 93.78% <0.00%> (-4.35%) ⬇️
module/zfs/vdev_raidz.c 89.21% <0.00%> (-4.25%) ⬇️
module/zfs/zio_compress.c 89.74% <0.00%> (-2.57%) ⬇️
module/zfs/lzjb.c 98.14% <0.00%> (-1.86%) ⬇️
cmd/ztest/ztest.c 78.79% <0.00%> (-1.38%) ⬇️
module/zfs/vdev_queue.c 94.80% <0.00%> (-1.23%) ⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ed4391...b10e392. Read the comment docs.

@allanjude
Copy link
Contributor

I know that some hypervisors also do this, to allow scaling up the number of CPUs without rebooting

@DeHackEd
Copy link
Contributor Author

DeHackEd commented May 3, 2020

Yes, but nr_cpus is the most that will be allowed. Adding more past this point won't be accepted by the kernel. However ZFS will create/allow additional threads at this higher number in the meantime and exceeding the number of CPUs won't be good for performance.

This is a bare metal machine that I am using.

@behlendorf
Copy link
Contributor

While systems which support hot plugging are rare, they do exist. My concern with this change is that it would cause such a system to panic when adding a cpu. Which isn't great.

Instead of changing max_cpus to be the same as boot_cpus, I think we should audit the max_ncpu users and check if they should really be using boot_cpus. In the case of arc_prune limiting it even further to just a couple threads would probably be best to avoid contention.

@DeHackEd
Use boot_ncpus in place of max_ncpus in taskq_create
d6bc06d 
Due to hotplug support or BIOS bugs sometimes max_ncpus can be
an absurdly high value. I have a system with 32 cores/threads
but reports max_ncpus == 440. This many threads potentially
cripples the system during arc_prune floods for example.

boot_ncpus is the number of working CPUs when called so use
that instead.

Signed-off-by: DHE <git@dehacked.net>
@DeHackEd
Copy link
Contributor Author

DeHackEd commented May 17, 2020 •
edited
Loading

I went over the code and see what you mean. Yeah hotplugging CPUs could cause.. umm.. problems.

I went ahead and changed any taskq that references max_ncpus to boot_ncpus instead, except in FreeBSD code where this doesn't seem to matter. The last remaining reference is in bsd's vdev_file.c.

I'm not qualified to provide an upper bound on arc_prune thread counts and just left it a boot_ncpus. At least it's not 14 times too high any more.

@DeHackEd DeHackEd changed the title Use online CPUs as max_ncpus Use online CPUs rather than max_ncpus for taskq thread count May 17, 2020
behlendorf
behlendorf approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf 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. Let me also refer you to PR #10331 where decreasing the number of prune threads has been proposed.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 20, 2020
@behlendorf behlendorf merged commit 57434ab into openzfs:master May 20, 2020
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
@DeHackEd @as-com
Use boot_ncpus in place of max_ncpus in taskq_create
30ce876 
Due to hotplug support or BIOS bugs sometimes max_ncpus can be
an absurdly high value. I have a system with 32 cores/threads
but reports max_ncpus == 440. This many threads potentially
cripples the system during arc_prune floods for example.

boot_ncpus is the number of working CPUs when called so use
that instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Closes openzfs#10282
(cherry picked from commit 57434ab)
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
@DeHackEd @as-com
Use boot_ncpus in place of max_ncpus in taskq_create
bc69650 
Due to hotplug support or BIOS bugs sometimes max_ncpus can be
an absurdly high value. I have a system with 32 cores/threads
but reports max_ncpus == 440. This many threads potentially
cripples the system during arc_prune floods for example.

boot_ncpus is the number of working CPUs when called so use
that instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Closes openzfs#10282
(cherry picked from commit 57434ab)
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 16, 2020
@DeHackEd @tonyhutter
Use boot_ncpus in place of max_ncpus in taskq_create
76a1232 
Due to hotplug support or BIOS bugs sometimes max_ncpus can be
an absurdly high value. I have a system with 32 cores/threads
but reports max_ncpus == 440. This many threads potentially
cripples the system during arc_prune floods for example.

boot_ncpus is the number of working CPUs when called so use
that instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Closes openzfs#10282
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
@DeHackEd @jsai20
Use boot_ncpus in place of max_ncpus in taskq_create
138e290 
Due to hotplug support or BIOS bugs sometimes max_ncpus can be
an absurdly high value. I have a system with 32 cores/threads
but reports max_ncpus == 440. This many threads potentially
cripples the system during arc_prune floods for example.

boot_ncpus is the number of working CPUs when called so use
that instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Closes openzfs#10282
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DeHackEd @codecov-io @allanjude @behlendorf