Clean up OS-specific ARC and kmem code #10499

ahrens · 2020-06-25T17:00:44Z

Motivation and Context

OS-specific code (e.g. under module/os/linux) does not need to share its code structure with any other operating systems. In particular, the ARC and kmem code need not be similar to the code in illumos, because we won't be syncing this OS-specific code between operating systems. For example, if/when illumos support is added to the common repo, we would add a file module/os/illumos/zfs/arc_os.c for the illumos versions of this code.

Therefore, we can simplify the code in the OS-specific ARC and kmem routines.

Description

These changes do not impact system behavior, they are purely code cleanup. The changes are:

Arenas are not used on Linux or FreeBSD (they are always NULL), so heap_arena, zio_arena, and zio_alloc_arena can be removed, along with code that uses them.

In arc_available_memory():

desfree is unused, remove it
rename freemem to avoid conflict with pre-existing #define
remove checks related to arenas
use units of bytes, rather than converting from bytes to pages and then back to bytes

SPL_KMEM_CACHE_REAP is unused, remove it.

skc_reap is unused, remove it.

The count argument to spl_kmem_cache_reap_now() is unused, remove it.

vmem_size() and associated type and macros are unused, remove them.

In arc_memory_throttle(), use a less confusing variable name to store the result of arc_free_memory().

Note: I've kept this as several commits to aid in reviewing; I'll collapse this to one commit once the review is approved.

How Has This Been Tested?

ZTS

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

ghost

Nice cleanup!

codecov · 2020-06-26T00:00:09Z

Codecov Report

Merging #10499 into master will decrease coverage by 0.08%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #10499      +/-   ##
==========================================
- Coverage   79.62%   79.53%   -0.09%     
==========================================
  Files         391      391              
  Lines      123908   123887      -21     
==========================================
- Hits        98656    98539     -117     
- Misses      25252    25348      +96

Flag	Coverage Δ
#kernel	`80.02% <75.00%> (-0.07%)`	⬇️
#user	`65.74% <ø> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
lib/libzpool/kernel.c	`67.81% <ø> (ø)`
module/os/linux/spl/spl-vmem.c	`100.00% <ø> (+45.83%)`	⬆️
module/zfs/arc.c	`89.17% <ø> (-0.27%)`	⬇️
module/os/linux/zfs/arc_os.c	`59.50% <66.66%> (+0.58%)`	⬆️
module/os/linux/spl/spl-kmem-cache.c	`75.44% <100.00%> (-9.67%)`	⬇️
module/zfs/vdev_indirect.c	`72.83% <0.00%> (-7.84%)`	⬇️
module/zfs/dsl_synctask.c	`92.40% <0.00%> (-2.54%)`	⬇️
lib/libzfs/libzfs_changelist.c	`84.37% <0.00%> (-1.96%)`	⬇️
module/zfs/vdev_mirror.c	`94.77% <0.00%> (-1.50%)`	⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94a2dca...a0f1427. Read the comment docs.

pzakha

Nice cleanup! It does seem like there's a small change in logic, perhaps it would be better to take it out to another review?

pzakha · 2020-06-26T20:09:44Z

module/os/linux/zfs/arc_os.c

-	 * the scanner doesn't start up while we're freeing memory.
-	 */
-	n = PAGESIZE * (realfree - sysfree - needfree);
+	n = arc_free_memory() - arc_sys_free;


It seems like you've removed arc_need_free from this calculation, was this intentional?

My thinking was that if arc_need_free is nonzero, we'll hit the case above and lowest will already be lower than what we could have here. On second examination, that's only true if arc_free_memory() - arc_sys_free is not negative. So I'll add arc_need_free back in here, just in case.

behlendorf · 2020-06-27T17:28:45Z

@ahrens would you mind squashing and rebasing this to resolve the conflict.

OS-specific code (e.g. under `module/os/linux`) does not need to share its code structure with any other operating systems. In particular, the ARC and kmem code need not be similar to the code in illumos, because we won't be syncing this OS-specific code between operating systems. For example, if/when illumos support is added to the common repo, we would add a file `module/os/illumos/zfs/arc_os.c` for the illumos versions of this code. Therefore, we can simplify the code in the OS-specific ARC and kmem routines. These changes do not impact system behavior, they are purely code cleanup. The changes are: Arenas are not used on Linux or FreeBSD (they are always `NULL`), so `heap_arena`, `zio_arena`, and `zio_alloc_arena` can be removed, along with code that uses them. In `arc_available_memory()`: * `desfree` is unused, remove it * rename `freemem` to avoid conflict with pre-existing `#define` * remove checks related to arenas * use units of bytes, rather than converting from bytes to pages and then back to bytes `SPL_KMEM_CACHE_REAP` is unused, remove it. `skc_reap` is unused, remove it. The `count` argument to `spl_kmem_cache_reap_now()` is unused, remove it. `vmem_size()` and associated type and macros are unused, remove them. In `arc_memory_throttle()`, use a less confusing variable name to store the result of `arc_free_memory()`. Signed-off-by: Matthew Ahrens <mahrens@delphix.com>

ahrens · 2020-06-29T04:34:06Z

@behlendorf done.

OS-specific code (e.g. under `module/os/linux`) does not need to share its code structure with any other operating systems. In particular, the ARC and kmem code need not be similar to the code in illumos, because we won't be syncing this OS-specific code between operating systems. For example, if/when illumos support is added to the common repo, we would add a file `module/os/illumos/zfs/arc_os.c` for the illumos versions of this code. Therefore, we can simplify the code in the OS-specific ARC and kmem routines. These changes do not impact system behavior, they are purely code cleanup. The changes are: Arenas are not used on Linux or FreeBSD (they are always `NULL`), so `heap_arena`, `zio_arena`, and `zio_alloc_arena` can be removed, along with code that uses them. In `arc_available_memory()`: * `desfree` is unused, remove it * rename `freemem` to avoid conflict with pre-existing `#define` * remove checks related to arenas * use units of bytes, rather than converting from bytes to pages and then back to bytes `SPL_KMEM_CACHE_REAP` is unused, remove it. `skc_reap` is unused, remove it. The `count` argument to `spl_kmem_cache_reap_now()` is unused, remove it. `vmem_size()` and associated type and macros are unused, remove them. In `arc_memory_throttle()`, use a less confusing variable name to store the result of `arc_free_memory()`. Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Pavel Zakharov <pavel.zakharov@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Closes openzfs#10499

ahrens requested review from behlendorf and a user June 25, 2020 17:00

ahrens added the Status: Code Review Needed Ready for review and testing label Jun 25, 2020

ghost approved these changes Jun 25, 2020

View reviewed changes

ahrens force-pushed the arc-cleanup branch from bfef970 to 9fd9d12 Compare June 26, 2020 02:37

grwilson approved these changes Jun 26, 2020

View reviewed changes

pzakha reviewed Jun 26, 2020

View reviewed changes

ahrens force-pushed the arc-cleanup branch from 9fd9d12 to a895db0 Compare June 26, 2020 20:35

pzakha approved these changes Jun 26, 2020

View reviewed changes

behlendorf approved these changes Jun 26, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 26, 2020

ahrens force-pushed the arc-cleanup branch from a895db0 to a0f1427 Compare June 29, 2020 04:33

behlendorf merged commit 3c42c9e into openzfs:master Jun 29, 2020

ahrens deleted the arc-cleanup branch June 29, 2020 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up OS-specific ARC and kmem code #10499

Clean up OS-specific ARC and kmem code #10499

ahrens commented Jun 25, 2020 •

edited

ghost left a comment

codecov bot commented Jun 26, 2020 •

edited

pzakha left a comment

pzakha Jun 26, 2020

ahrens Jun 26, 2020

behlendorf commented Jun 27, 2020

ahrens commented Jun 29, 2020

Clean up OS-specific ARC and kmem code #10499

Clean up OS-specific ARC and kmem code #10499

Conversation

ahrens commented Jun 25, 2020 • edited

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ghost left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2020 • edited

Codecov Report

pzakha left a comment

Choose a reason for hiding this comment

pzakha Jun 26, 2020

Choose a reason for hiding this comment

ahrens Jun 26, 2020

Choose a reason for hiding this comment

behlendorf commented Jun 27, 2020

ahrens commented Jun 29, 2020

ahrens commented Jun 25, 2020 •

edited

codecov bot commented Jun 26, 2020 •

edited