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

Move gethrtime() calls out of vdev queue lock. #12281

Merged
merged 1 commit into from
Jul 6, 2021
Merged

Move gethrtime() calls out of vdev queue lock. #12281

merged 1 commit into from
Jul 6, 2021

Conversation

amotin
Copy link
Member

@amotin amotin commented Jun 25, 2021

This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters. With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts. Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

How Has This Been Tested?

On 40-thread FreeBSD system switched to use ACPI timecounter instead of TSC on heavy uncached 4KB ZVOLs reads profiler shows reduction of CPU time spent spinning on the vdev queue lock from 25% to almost zero.

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
Move gethrtime() calls out of vdev queue lock.
d7039a0 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jun 25, 2021
@amotin amotin requested review from grwilson and ahrens June 25, 2021 02:28
ahrens
ahrens approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@ahrens ahrens left a comment

Choose a reason for hiding this comment

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

This seems fine. Out of curiosity, what are the most common systems that don't have a fast gethrtime() by default? Or are people disabling TSC for some reason?

module/zfs/vdev_queue.c Show resolved Hide resolved
@amotin
Copy link
Member Author

amotin commented Jun 25, 2021 •
edited
Loading

There were plenty of systems with problematic TSC over the years: changing rate, stopping in deep idle, not coherent on SMP, slow in VMs, etc. Last time it is better, but why depend on something if it can be avoided? And even "fast" gethrtime() I see in profiles.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 29, 2021
@behlendorf behlendorf merged commit 97752ba into openzfs:master Jul 6, 2021
tle211212 pushed a commit to tle211212/zfs that referenced this pull request Jul 9, 2021
@amotin @tle211212
Move gethrtime() calls out of vdev queue lock
db5624a 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12281
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 23, 2021
@amotin @behlendorf
Move gethrtime() calls out of vdev queue lock
7bb255b 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes openzfs#12281
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
@amotin @behlendorf
Move gethrtime() calls out of vdev queue lock
c12675f 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes openzfs#12281
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
@amotin
Move gethrtime() calls out of vdev queue lock
2d08bb2 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes openzfs#12281
@amotin amotin deleted the gethrtime_vq branch August 24, 2021 20:18
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
@amotin @behlendorf
Move gethrtime() calls out of vdev queue lock
afa0a29 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes openzfs#12281
behlendorf pushed a commit that referenced this pull request Aug 31, 2021
@amotin @behlendorf
Move gethrtime() calls out of vdev queue lock
3b3b893 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes #12281
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2021
@amotin @tonyhutter
Move gethrtime() calls out of vdev queue lock
41b33dc 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Closes openzfs#12281
datacore-rm pushed a commit to DataCoreSoftware/openzfs that referenced this pull request Apr 1, 2022
@amotin @datacore-rm
Move gethrtime() calls out of vdev queue lock
7c9ede0 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12281
datacore-rm pushed a commit to DataCoreSoftware/openzfs that referenced this pull request Apr 1, 2022
@amotin @datacore-rm
Move gethrtime() calls out of vdev queue lock
c12f90a 
This dramatically reduces the lock contention on systems with slower
(non-TSC) timecounters.  With TSC the difference is minimal, but since
this lock is pretty congested, any improvement counts.  Plus I don't
see any reason to do it under the lock other than the latency of the
lock itself, which this change actually reduces.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@ahrens ahrens ahrens approved these changes

@grwilson grwilson Awaiting requested review from grwilson

Assignees

@mmaybee mmaybee

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 @mmaybee