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.

Sign up for GitHub

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

Jump to bottom

Couple minor ARC optimizations. #12348

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Couple minor ARC optimizations. #12348

merged 1 commit into from
Jul 20, 2021

Conversation

amotin
Copy link
Member

@amotin amotin commented Jul 11, 2021

Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.). It removes unneeded
(even though usually cached) pointer dereferences from many places.

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation. Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

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:

@amotin amotin added Type: Performance Performance improvement or performance problem Status: Code Review Needed Ready for review and testing labels Jul 11, 2021
@amotin amotin requested a review from behlendorf July 11, 2021 04:03
@amotin
Couple minor ARC optimizations.
4e78a27 
Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.).  It removes unneeded
(even though usually cached) pointer dereferences from many places.

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation.  Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

Signed-off-by: Alexander Motin <[email protected]>
@mmaybee mmaybee added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 16, 2021
ahrens
ahrens approved these changes Jul 16, 2021
View reviewed changes
include/sys/arc_impl.h
@@ -964,6 +964,13 @@ typedef struct arc_evict_waiter {
#define arc_c_max ARCSTAT(arcstat_c_max) /* max target cache size */
#define arc_sys_free ARCSTAT(arcstat_sys_free) /* target system free bytes */

#define arc_anon (&ARC_anon)
#define arc_mru (&ARC_mru)
#define arc_mru_ghost (&ARC_mru_ghost)
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 this style of macros make the code harder to read. Since they are already all over the ARC, I'm OK with the change as-is, but if you or anyone else would like to improve the code, it would be great to either:

  • change all the users to do &ARC_anon, etc. instead of arc_anon

or

  • make these inline functions, like inline arc_state_t* arc_anon() {return (&ARC_anon);)

@mmaybee mmaybee merged commit 8172df6 into openzfs:master Jul 20, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
@amotin
Minor ARC optimizations
0976a5e 
Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.). 

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation.  Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Mark Maybee <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes openzfs#12348
@amotin amotin deleted the arc_states branch August 24, 2021 20:17
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
@amotin @behlendorf
Minor ARC optimizations
d790c5c 
Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.).

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation.  Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Mark Maybee <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes openzfs#12348
behlendorf pushed a commit that referenced this pull request Aug 31, 2021
@amotin @behlendorf
Minor ARC optimizations
a633ee7 
Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.).

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation.  Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Mark Maybee <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #12348
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2021
@amotin @tonyhutter
Minor ARC optimizations
6a49948 
Remove unneeded global, practically constant, state pointer variables
(arc_anon, arc_mru, etc.), replacing them with macros of real state
variables addresses (&ARC_anon, &ARC_mru, etc.).

Change ARC_EVICT_ALL from -1ULL to UINT64_MAX, not requiring special
handling in inner loop of ARC reclamation.  Respectively change bytes
argument of arc_evict_state() from int64_t to uint64_t.

Reviewed-by: Matthew Ahrens <[email protected]>
Reviewed-by: Mark Maybee <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes openzfs#12348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmaybee mmaybee mmaybee approved these changes

@ahrens ahrens ahrens approved these changes

@behlendorf behlendorf Awaiting requested review from behlendorf

Assignees

@mmaybee mmaybee

Labels
Status: Accepted Ready to integrate (reviewed, tested) Type: Performance Performance improvement or performance problem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amotin @ahrens @mmaybee