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

Run arc_evict thread at higher priority #12151

Closed
wants to merge 1 commit into from
Closed

Run arc_evict thread at higher priority #12151

wants to merge 1 commit into from

Conversation

tonynguien
Copy link
Contributor

Motivation and Context

Running arc_evict thread at higher priority minimizes it getting
scheduled off CPU and can improve performance for workload with high
ARC evict activities.

On mixed read/write and sequential read workloads, I've seen between
10-20% better performance.

Description

Extending zthr_create_timer() to take a new priority_t argument and
arc_evict_zthr is created with highest priority value, maxclsyspri.

How Has This Been Tested?

Manual tests with sequential reads and mixed read/writes with larger pool size.

ZFS performance test suite also show better though smaller gains for
sequential read and sequential writes, 8% and 3%, respectively.

Test setup:

  • 8 vCPU
  • 32 GB memory
  • 4 x 100 SSD disks

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Signed-off-by: Tony Nguyen <tony.nguyen@delphix.com>
@tonynguien
Copy link
Contributor Author

Attaching offcpu flame graphs showing how much arc_evict thread was involuntarily context switched off CPU and that was minimized with arc_evict running at higher priority.

arc_evict.tar.gz

@adamdmoss
Copy link
Contributor

Would setting CPU affinity for the eviction thread have the same effect with more consistency?
(I'm a little worried about interactive performance whenever a zfs thread with nontrivial work gets high priority - had to patch-out these cases in the past - would like to see this boosted thread priority as a last resort or a nondefault tuning.)

@amotin
Copy link
Member

amotin commented May 29, 2021

Would setting CPU affinity for the eviction thread have the same effect with more consistency?

@adamdmoss How would any affinity made thread to run faster? It would likely do opposite by limiting scheduler choices.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 30, 2021
@tonynguien
Copy link
Contributor Author

Would setting CPU affinity for the eviction thread have the same effect with more consistency?
(I'm a little worried about interactive performance whenever a zfs thread with nontrivial work gets high priority - had to patch-out these cases in the past - would like to see this boosted thread priority as a last resort or a nondefault tuning.)

Setting CPU affinity wouldn't help since the thread still get taken off CPU as often and may even hurt performance because setting would limit which CPU the thread can run (less run time).

When arc_evict thread is active (evicting), I/Os may be stuck waiting until arc_evict frees up enough memory so it's important that the thread gets needed CPU time to complete its work.

I'm not opposed to providing a tunable when we see a problem. I hope we can avoid an unnecessary tunable.

@tonynguien
Copy link
Contributor Author

I need to do more testing.

Using defsysclprio, i.e. nice=0, is giving similar performance improvement and should reduce CPU starvation concerns. I'll run more tests.

@tonynguien
Copy link
Contributor Author

I couldn't update this due to new repo. I filed #12397 and closing this.

@tonynguien tonynguien closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants