Fix ARC behavior on 32-bit systems #6734
Merged
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
behlendorf
force-pushed
the
async-destroy
branch
3 times, most recently
from
323b6bf
to
5e09305
Compare
Codecov Report
@@ Coverage Diff @@
## master #6734 +/- ##
==========================================
+ Coverage 74.12% 74.13% +<.01%
==========================================
Files 295 295
Lines 93865 93866 +1
==========================================
+ Hits 69581 69584 +3
+ Misses 24284 24282 -2
Continue to review full report at Codecov.
|
behlendorf
force-pushed
the
async-destroy
branch
2 times, most recently
from
779ef98
to
e68dfec
Compare
|
Testing Update:
|
behlendorf
force-pushed
the
async-destroy
branch
from
e68dfec
to
42d6e6f
Compare
|
Related to #5352 |
dpquigl
approved these changes
Oct 10, 2017
module/zfs/arc.c
Outdated
| #endif /* ZFS_GLOBAL_NODE_PAGE_STATE */ | ||
| #endif /* CONFIG_HIGHMEM */ | ||
| #else | ||
| return (spa_get_random(arc_all_memory() * 20 / 100)); | ||
| #endif |
There was a problem hiding this comment.
Maybe add a /* KERNEL */ here just to make it clearer to read. I had a hard time figuring out what the spa_get_random was for.
| @@ -6947,7 +6971,7 @@ arc_memory_throttle(uint64_t reserve, uint64_t txg) | |||
| * continue to let page writes occur as quickly as possible. | |||
| */ | |||
| if (current_is_kswapd()) { | |||
| if (page_load > MAX(ptob(minfree), available_memory) / 4) { | |||
| if (page_load > MAX(arc_sys_free / 4, available_memory) / 4) { | |||
There was a problem hiding this comment.
These two numbers seem very different. One is the number of free pages in bytes the other seems to be the number of 32 bit words? Is this really the comparison?
There was a problem hiding this comment.
All of these values are in fact in bytes and this PR doesn't make any real change here. It only removes the minfree local and the needless btop() -> ptob() conversion. This is logic was adapted from OpenZFS and is intended to act as a throttle when there is significant memory pressure.
That said, according the codecov report this page_load block was never run during the ZTS. I find that pretty suspicious so we should revisit this, bit it may be that we simply never put enough memory pressure on the system. But let's address that in a different PR if needed so we can avoid making any functional changes in this one.
behlendorf
force-pushed
the
async-destroy
branch
from
42d6e6f
to
509c194
Compare
With the addition of the ABD changes consumption of the virtual address space has been greatly reduced. This exposed an issue on CONFIG_HIGHMEM systems where free memory was being calculated incorrectly. Functionally this didn't cause any major problems prior to ABD because a lack of available virtual address space was used as an indicator of low memory. This patch makes the following changes to address the issue and in the process realigns the code further with OpenZFS. There are no substantive changes in behavior for 64-bit systems. * Added CONFIG_HIGHMEM case to the arc_all_memory() and arc_free_memory() functions to only consider low memory pages on CONFIG_HIGHMEM systems. * The arc_free_memory() function was updated to return bytes instead of pages to be consistent with the other helper functions. In user space we make up some reasonable values since currently only testing is performed in this context. * Adds three new values to the arcstats kstat to provide visibility in to the ARC's assessment of the memory situation: memory_all_bytes, memory_free_bytes, and memory_available_bytes. * Added kmem_reap() call to arc_available_memory() for 32-bit builds to realign code with OpenZFS. * Reduced size of test file in /async_destroy_001_pos.ksh to speed up test case. Multiple txgs are still required. * Move vdevs used by zpool_clear_001_pos and zpool_upgrade_002_pos to TEST_BASE_DIR location to speed up test cases. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#5352
aerusso
pushed a commit
to aerusso/zfs
that referenced
this pull request
Oct 11, 2017
With the addition of the ABD changes consumption of the virtual address space has been greatly reduced. This exposed an issue on CONFIG_HIGHMEM systems where free memory was being calculated incorrectly. Functionally this didn't cause any major problems prior to ABD because a lack of available virtual address space was used as an indicator of low memory. This patch makes the following changes to address the issue and in the process realigns the code further with OpenZFS. There are no substantive changes in behavior for 64-bit systems. * Added CONFIG_HIGHMEM case to the arc_all_memory() and arc_free_memory() functions to only consider low memory pages on CONFIG_HIGHMEM systems. * The arc_free_memory() function was updated to return bytes instead of pages to be consistent with the other helper functions. In user space we make up some reasonable values since currently only testing is performed in this context. * Adds three new values to the arcstats kstat to provide visibility in to the ARC's assessment of the memory situation: memory_all_bytes, memory_free_bytes, and memory_available_bytes. * Added kmem_reap() call to arc_available_memory() for 32-bit builds to realign code with OpenZFS. * Reduced size of test file in /async_destroy_001_pos.ksh to speed up test case. Multiple txgs are still required. * Move vdevs used by zpool_clear_001_pos and zpool_upgrade_002_pos to TEST_BASE_DIR location to speed up test cases. Reviewed-by: David Quigley <david.quigley@intel.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#5352 Closes openzfs#6734
tonyhutter
pushed a commit
that referenced
this pull request
Oct 16, 2017
With the addition of the ABD changes consumption of the virtual address space has been greatly reduced. This exposed an issue on CONFIG_HIGHMEM systems where free memory was being calculated incorrectly. Functionally this didn't cause any major problems prior to ABD because a lack of available virtual address space was used as an indicator of low memory. This patch makes the following changes to address the issue and in the process realigns the code further with OpenZFS. There are no substantive changes in behavior for 64-bit systems. * Added CONFIG_HIGHMEM case to the arc_all_memory() and arc_free_memory() functions to only consider low memory pages on CONFIG_HIGHMEM systems. * The arc_free_memory() function was updated to return bytes instead of pages to be consistent with the other helper functions. In user space we make up some reasonable values since currently only testing is performed in this context. * Adds three new values to the arcstats kstat to provide visibility in to the ARC's assessment of the memory situation: memory_all_bytes, memory_free_bytes, and memory_available_bytes. * Added kmem_reap() call to arc_available_memory() for 32-bit builds to realign code with OpenZFS. * Reduced size of test file in /async_destroy_001_pos.ksh to speed up test case. Multiple txgs are still required. * Move vdevs used by zpool_clear_001_pos and zpool_upgrade_002_pos to TEST_BASE_DIR location to speed up test cases. Reviewed-by: David Quigley <david.quigley@intel.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #5352 Closes #6734
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.
Description
With the addition of the ABD changes consumption of the virtual address space has been greatly reduced. This exposed an issue on CONFIG_HIGHMEM systems where free memory was being calculated incorrectly. Functionally this didn't cause any major problems prior to ABD because a lack of available virtual address space was used as an indicator of low memory.
This patch makes the following changes to address the issue andin the process realigns the code further with OpenZFS. There are no substantive changes in behavior for 64-bit systems.
Added CONFIG_HIGHMEM case to the arc_all_memory() and arc_free_memory() functions to only consider low memory pages on CONFIG_HIGHMEM systems.
The arc_free_memory() function was updated to return bytes instead of pages to be consistent with the other helper functions. In user space we make up some reasonable values since currently only testing is performed in this context.
Adds three new values to the arcstats kstat to provide visibility in to the ARC's assessment of the memory situation: memory_all_bytes, memory_free_bytes, and memory_available_bytes.
Added kmem_reap() call to arc_available_memory() for 32-bit builds to realign code with OpenZFS.
Reduced size of test file in /async_destroy_001_pos.ksh to speed up test case. Multiple txgs are still required.
Move vdevs used by zpool_clear_001_pos and zpool_upgrade_002_pos to TEST_BASE_DIR location to speed up test cases.
Motivation and Context
Resolve 32-bit testing failures when running the ZTS. The async_destroy_001_pos test was particularly good at triggering a low memory situation which could trigger the OOM killer. More broadly speaking these changes should bring increased stability to 32-bit platforms. Tuning is still required to achieve good performance when operating with an ARC that is only 100MB in size.
How Has This Been Tested?
Locally by running various stress tests and observing the system.
Types of changes
Checklist:
Signed-off-by.