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

Implement parallel ARC eviction #16486

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

Conversation

allanjude
Copy link
Contributor

Sponsored-by: Expensify, Inc.
Sponsored-by: Klara, Inc.

Motivation and Context

Read and write performance can become limited by the arc_evict process being single threaded.
Additional data cannot be added to the ARC until sufficient existing data is evicted.

On many-core systems with TBs of RAM, a single thread becomes a significant bottleneck.

With the change we see a 25% increase in read and write throughput

Description

Use a new taskq to run multiple multiple arc_evict() threads at once, each given a fraction of the desired memory to reclaim

How Has This Been Tested?

Benchmarking with a full ARC to measure the performance difference.

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:

Comment on lines +4172 to +4181
uint64_t nchunks = ((left - 1) >> MIN_EVICT_PERTASK_SHIFT) + 1;
unsigned n = nchunks < num_sublists ? nchunks : num_sublists;
uint64_t fullrows = nchunks / n;
unsigned lastrowcols = nchunks % n;
unsigned k = (lastrowcols ? lastrowcols : n);

uint64_t bytes_pertask_low =
fullrows << MIN_EVICT_PERTASK_SHIFT;
uint64_t bytes_pertask = bytes_pertask_low + (lastrowcols ?
(1 << MIN_EVICT_PERTASK_SHIFT) : 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are over-engineering here. I don't think eviction per taskq should really be a multiple of 1 << MIN_EVICT_PERTASK_SHIFT to complicate the logic, merely it should be bigger than one. So you could just use MIN_EVICT_PERTASK_SHIFT to decide number of tasks, and then split the eviction amount equally between them.

And I wonder if it would make sense to scale number of tasks with eviction not linearly, but in some logarithimic fashion to not spin too many threads at once, stressing the system more for diminishing return.

uint64_t evict = i < k ? bytes_pertask :
bytes_pertask_low;

ASSERT3S(n, <=, num_sublists);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is true, I think it is irrelevant here.


ASSERT3S(n, <=, num_sublists);

memset(&evarg[i].tqe, 0, sizeof (evarg[i].tqe));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is taskq_init_ent() for this.

module/zfs/arc.c Outdated
@@ -7737,12 +7871,19 @@ arc_init(void)

buf_init();

if (zfs_arc_evict_threads == 0)
zfs_arc_evict_threads = MIN(16, max_ncpus >> 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is subjective, but IMHO 1/2 of CPUs for memory eviction is too much if system is doing anything else. And the current algorithm for non-trivial eviction would tend to use all of them quite often, stressing the system.

Read and write performance can become limited by the arc_evict
process being single threaded. Additional data cannot be added
to the ARC until sufficient existing data is evicted.

On many-core systems with TBs of RAM, a single thread becomes
a significant bottleneck.

With the change we see a 25% increase in read and write throughput

Sponsored-by: Expensify, Inc.
Sponsored-by: Klara, Inc.
Co-authored-by: Allan Jude <[email protected]>
Co-authored-by: Mateusz Piotrowski <[email protected]>
Signed-off-by: Alexander Stetsenko <[email protected]>
Signed-off-by: Allan Jude <[email protected]>
Signed-off-by: Mateusz Piotrowski <[email protected]>
@adamdmoss
Copy link
Contributor

adamdmoss commented Sep 12, 2024

I've been casually testing this out (combined with the parallel_dbuf_evict PR) over the last couple of weeks (most recently, 5b070d1 ).

I've not been hammering it hard or specifically, just letting it do its thing with my messing-around desktop system.

Hit a probable regression today, though: while mv'ing a meager 8GB of files from one pool to another, all my zfs IO got really high-latency, and an iotop showed that the copy part of the move (this being a mv across pools, so in reality it's a copy-and-remove) was running at a painful few 100KB/sec, and the zfs arc_evict thread was taking a whole core... but just one core.

In time it all cleared up and of course I can't conclusively blame this PR's changes, but I left with two fuzzy observations:

  • In many years of mucking around with ZFS I've never(?) seemed to get the 'arc_evict is pegging CPU badly' issue until I started testing this PR's changes (though I'm aware the issue occurs in the wild for folks on master/release ZFSes)
  • arc_evict was only using one core as far as I can tell, so I guess the parallelism which is the point of this PR just wasn't kicking-in for some reason anyway and/or the spinning was happening outside of the parallelized part

@0mp
Copy link
Contributor

0mp commented Sep 12, 2024

I have updated the patch with a different logic for picking the default maximum number of ARC eviction threads. The new logic aims to pick the number that is one-eighth of the available CPUs, with a minimum of 2 and a maximum of 16.

@amotin
Copy link
Member

amotin commented Sep 12, 2024

one-eighth of the available CPUs, with a minimum of 2 and a maximum of 16.

Why would we need two evict threads on a single-core system? In that case I would probably prefer to disable taskqs completely. If that is a way to make it more logarithmic, then I would think about highbit(), though then it will grow pretty slow for very large systems, so that the limit of 16 will never be reached. But I am not exactly sure the faster growth would make sense, since it may cause more lock contentions in memory allocator, etc.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 13, 2024
@allanjude
Copy link
Contributor Author

one-eighth of the available CPUs, with a minimum of 2 and a maximum of 16.

Why would we need two evict threads on a single-core system? In that case I would probably prefer to disable taskqs completely. If that is a way to make it more logarithmic, then I would think about highbit(), though then it will grow pretty slow for very large systems, so that the limit of 16 will never be reached. But I am not exactly sure the faster growth would make sense, since it may cause more lock contentions in memory allocator, etc.

Right now, this is only enabled by a separate tunable, to enable multiple threads. So for the single CPU case, we don't expect it to be enabled. But for something like 4-12 core systems, we would want it to use at least 2 threads, and then grow from there, reaching 16 threads at 128 cores.

@amotin
Copy link
Member

amotin commented Sep 16, 2024

Right now, this is only enabled by a separate tunable, to enable multiple threads. So for the single CPU case, we don't expect it to be enabled.

Now that you mentioned it, I've noticed its been disabled by default. I don't like the idea to tune it manually in production depending on system size. I would prefer to to have reasonable automatic defaults.

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