Remove refcount from spa_config_*(). #12287
Conversation
|
I haven't removed the tag argument to not modify all the callers in case somebody may still need to restore the tracking for some debugging. I could make the use of refcount conditional, if people prefer, just didn't want to pollute the code. |
I agree that making this conditional would ugly up the code some, but I think it's still probably preferable. Making it conditional on On a related note I noticed that a lot of the |
|
OK. I've restored the debugging code under ifdef ZFS_SPA_CONFIG_DEBUG. Using ZFS_DEBUG seems not very useful, since the tracking was still disabled there, requiring code modification to be useful. Now single ZFS_SPA_CONFIG_DEBUG defined should do the trick in case somebody needs it. |
amotin
force-pushed
the
spa_config
branch
4 times, most recently
from
5d9a9b2 to
7580279
Compare
|
Switched it to ZFS_DEBUG so that code would be built and not break silently one day. Though it is a waste of resources. |
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc.
|
I've changed my mind. I don't want to over-engineer this. I don't want to introduce another full-fledged counter primitive to only be used in single piece of code that we can never enable even in debug build. Leaving the tag arguments in spa_config_*() functions should be sufficient for somebody who really needs to debug some reference leak of specific kind to just use DTrace on regular debug kernel, or at worst quickly reconstruct this tracking. Lets keep it simple. |
mmaybee
added
Status: Accepted
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#12287
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Closes openzfs#12287
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Closes #12287
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#12287
The only reason for spa_config_*() to use refcount instead of simple non-atomic (thanks to scl_lock) variable for scl_count is tracking, hard disabled for the last 8 years. Switch to simple int scl_count reduces the lock hold time by avoiding atomic, plus makes structure fit into single cache line, reducing the locks contention. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes #12287
The only reason for spa_config_*() to use refcount instead of simple
non-atomic (thanks to scl_lock) variable for scl_count is tracking,
hard disabled for the last 8 years. Switch to simple int scl_count
reduces the lock hold time by avoiding atomic, plus makes structure
fit into single cache line, reducing the locks contention.
How Has This Been Tested?
On 80-thread FreeBSD system doing ~630K mostly uncached 4KB ZVOLs reads profiler shows reduction of CPU time spent in spa_config_*() from 3.8% to 1.6%.
Types of changes
Checklist:
Signed-off-by.