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

Avoid extra taskq_dispatch() calls by DMU. #8909

Merged
merged 1 commit into from
Jun 25, 2019
Merged

Avoid extra taskq_dispatch() calls by DMU. #8909

merged 1 commit into from
Jun 25, 2019

Conversation

amotin
Copy link
Member

@amotin amotin commented Jun 15, 2019

Motivation and Context

DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes. Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually list any dnodes.

Description

This change adds very quick lockless check for empty sublists to avoid
this. Locking is not needed there, since the multilists are stable at
that time.

I also removed existing locking from multilist_is_empty(), since it any
way provides no any guaranties, unless multilist is stable, in which
case locking is not needed. Even if the multilist is not stable, it
is not critical for the list implementation, since list_is_empty() only
compares one variable with constant, and the worst that can happen is
it read some stale value, that may happen any way, unless caller locked
all the sublists explicitly first.

How Has This Been Tested?

Without the patch simple DTrace script on FreeBSD head on 72-core system shown tons of the tasks being dispatched on each dmu_objset_sync(). With the patch, if only small set of dnodes is modified (ZVOL or single file), it shows only few.

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:

kithrup
kithrup reviewed Jun 15, 2019
View reviewed changes
module/zfs/multilist.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 15, 2019
@codecov
Copy link

codecov bot commented Jun 15, 2019 •
edited
Loading

Codecov Report

Merging #8909 into master will decrease coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8909      +/-   ##
==========================================
- Coverage   78.61%   78.59%   -0.02%     
==========================================
  Files         388      388              
  Lines      120071   120087      +16     
==========================================
- Hits        94388    94383       -5     
- Misses      25683    25704      +21
Flag Coverage Δ
#kernel 79.48% <85.71%> (+0.03%) ⬆️
#user 66.13% <66.66%> (+0.1%) ⬆️

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 8f12a4f...caae5ea. Read the comment docs.

@behlendorf behlendorf requested a review from prakashsurya June 19, 2019 18:37
@amotin
Avoid extra taskq_dispatch() calls by DMU.
caae5ea 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Signed-off-by:  Alexander Motin <[email protected]>
@amotin
Copy link
Member Author

amotin commented Jun 24, 2019

OK. Considering the objection I've remade the patch, keeping full locking in place. It probably does not make much sense to fight too much, safety is better.

ahrens
ahrens approved these changes Jun 24, 2019
View reviewed changes
include/sys/multilist.h
@@ -89,6 +89,8 @@ void multilist_sublist_insert_head(multilist_sublist_t *, void *);
void multilist_sublist_insert_tail(multilist_sublist_t *, void *);
void multilist_sublist_move_forward(multilist_sublist_t *mls, void *obj);
void multilist_sublist_remove(multilist_sublist_t *, void *);
int multilist_sublist_is_empty(multilist_sublist_t *);
int multilist_sublist_is_empty_idx(multilist_t *, unsigned int);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these return boolean_t?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a difference. I just copied multilist_is_empty().

behlendorf
behlendorf approved these changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 24, 2019
@behlendorf behlendorf merged commit fc75467 into openzfs:master Jun 25, 2019
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 13, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
0e07b8d 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes openzfs#8909
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 22, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
212f714 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes openzfs#8909
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
067138a 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes openzfs#8909
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
eae1159 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes openzfs#8909
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
5fcecef 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes openzfs#8909
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
@amotin @tonyhutter
Avoid extra taskq_dispatch() calls by DMU
04d4df8 
DMU sync code calls taskq_dispatch() for each sublist of os_dirty_dnodes
and os_synced_dnodes.  Since the number of sublists by default is equal
to number of CPUs, it will dispatch equal, potentially large, number of
tasks, waking up many CPUs to handle them, even if only one or few of
sublists actually have any work to do.

This change adds check for empty sublists to avoid this.

Reviewed by: Sean Eric Fagan <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Closes #8909
@amotin amotin deleted the sync_tq branch August 24, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kithrup kithrup kithrup left review comments

@ahrens ahrens ahrens approved these changes

@behlendorf behlendorf behlendorf approved these changes

@prakashsurya prakashsurya Awaiting requested review from prakashsurya

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amotin @behlendorf @ahrens @kithrup