Possible txg locking improvements #4333
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryao
force-pushed
the
locking
branch
6 times, most recently
from
7c52fff to
aafb5d6
Compare
Using a mutex to protect increment and decrement operations is inefficient. It uses two atomic instructions, will spin when a single waiter is present (under Linux with typical configuration options) and if two waiters occur, will actually deschedule them, which is far slower than waiting. Using atomic instructions allows hardware to handle the blocking, which is much faster than software. Signed-off-by: Richard Yao <[email protected]>
There does not appear to be anything protected by ->tc_open_lock that callers use while holding ->tc_open_lock means that anything that preempts us would block on us when the only things that really need protection are ->tx_open_txg and ->tc_count. Signed-off-by: Richard Yao <[email protected]>
ryao
force-pushed
the
locking
branch
2 times, most recently
from
16db03c to
70d70fe
Compare
|
@behlendorf Thinking some more about the "txg visibility code should not execute under tc_open_lock" patch some more, direct reclaim in the memory allocation in Moving it out of the critical section is an improvement, although I suspect it does not go far enough as blocking here means that the sync thread cannot get new transactions to sync. I suspect that the right thing to do might be to change the code to use |
The memory allocation and locking in `spa_txg_history_*()` can potentially block txg_hold_open for arbitrarily long periods of time. Signed-off-by: Richard Yao <[email protected]>
…SEQID use in preemptible context" This reverts commit 15a9e03 and commit 8878261. Wrapping CPU_SEQID with kpreempt_disable() and kpreempt_enable() causes us to call preempt_schedule(). This allows the thread to be rescheduled on another CPU, allowing the vacated CPU to begin using the per-CPU structure that we still reference on the CPU we just vacated. This is an unnecessary source of contention that is a regression from the illumos behavior. We can turn off the Linux warnings by calling `raw_smp_processor_id()` instead of wrapping things with kpreempt_disable() and kpreempt_enable(), so lets do that. Requires-spl: refs/pull/532/head Signed-off-by: Richard Yao <[email protected]>
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this pull request
Jul 28, 2016
The memory allocation and locking in `spa_txg_history_*()` can potentially block txg_hold_open for arbitrarily long periods of time. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#4333
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This contains a few ideas that I had when looking at code related to
txg_quiesce().->tc_countrather than protecting it with a mutex. We have fewer points in the code where a mutex would block us and use 1 less atomic instruction in each.->tc_open_lockwhen processing a DMU transaction provided that we we have incremented->tc_count, so we let it go early.kpreempt_enable()defeats the purpose of usingCPU_SEQIDto index into a per-CPU structure because we are then preempted, allowing another thread to contend with us on the per-CPU structure we reference. This behavior was introduced by 15a9e03 as a means to work around scary warnings from Linux, but we can workaround them by mapping things to raw_smp_processor_id(), so lets do that to avoid unnecessary preemption.