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.

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

Upstream: add missing thread_exit() #10314

Merged
merged 1 commit into from
May 14, 2020

Conversation

lundman
Copy link
Contributor

@lundman lundman commented May 10, 2020

When a thread exits it is supposed to call thread_exit(). A few recent changes have missed a few calls to thread_exit(). I went through all thread_create() calls in the common code, but additional checks could be done in the module/os/$platform/ source files as well.

Motivation and Context

On some platforms, it is only semantically correct, but for example on macOS platform it is
required, both for freeing of memory, and for XNU thread termination.

Description

Adds missing thread_exit() calls and the end of a thread function.

How Has This Been Tested?

It compiles! Currently I am not at a stage where I can test each one, but hoping the CI tester will highlight any issues.

Types of changes

  • [ x] 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:

  • [ x] My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • [ x] 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.
  • [ x] All commit messages are properly formatted and contain Signed-off-by.

@codecov-io
Copy link

codecov-io commented May 11, 2020

Codecov Report

Merging #10314 into master will decrease coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10314      +/-   ##
==========================================
- Coverage   79.67%   79.33%   -0.34%     
==========================================
  Files         390      390              
  Lines      123336   123340       +4     
==========================================
- Hits        98263    97856     -407     
- Misses      25073    25484     +411     
Flag Coverage Δ
#kernel 79.86% <100.00%> (-0.31%) ⬇️
#user 64.76% <50.00%> (-1.38%) ⬇️
Impacted Files Coverage Δ
module/zfs/dmu_send.c 84.63% <100.00%> (+0.01%) ⬆️
module/zfs/vdev_initialize.c 97.78% <100.00%> (-0.32%) ⬇️
module/zfs/vdev_removal.c 94.36% <100.00%> (-2.30%) ⬇️
module/zfs/vdev_trim.c 96.38% <100.00%> (-0.18%) ⬇️
module/zfs/vdev_indirect.c 74.00% <0.00%> (-10.34%) ⬇️
module/os/linux/spl/spl-kmem-cache.c 75.58% <0.00%> (-9.23%) ⬇️
cmd/ztest/ztest.c 74.59% <0.00%> (-7.72%) ⬇️
cmd/zvol_id/zvol_id_main.c 76.31% <0.00%> (-5.27%) ⬇️
module/zfs/aggsum.c 95.34% <0.00%> (-4.66%) ⬇️
module/os/linux/zfs/arc_os.c 59.67% <0.00%> (-3.23%) ⬇️
... and 53 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 7fcf824...d5f648b. Read the comment docs.

@mattmacy
Copy link
Contributor

FYI I dealt with this by creating a wrapper for thread dispatch:
https://github.com/openzfs/zfs/blob/master/include/os/freebsd/spl/sys/proc.h#L72

@ceedriic
Copy link
Contributor

ceedriic commented May 11, 2020

FYI I dealt with this by creating a wrapper for thread dispatch:
https://github.com/openzfs/zfs/blob/master/include/os/freebsd/spl/sys/proc.h#L72

Why did you make it __inline? on purpose or by mistake?

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 11, 2020
@behlendorf
Copy link
Contributor

Let's be consistent about this. Since the user space and Linux versions of thread_create() expect the thread_exit() call in the thread function, any objection to removing the kthread_exit() call from the FreeBSD wrapper as part of this change?

@mattmacy
Copy link
Contributor

Let's be consistent about this. Since the user space and Linux versions of thread_create() expect the thread_exit() call in the thread function, any objection to removing the kthread_exit() call from the FreeBSD wrapper as part of this change?

The wrapper is no longer needed if you fix the client code.

@mattmacy
Copy link
Contributor

FYI I dealt with this by creating a wrapper for thread dispatch:
https://github.com/openzfs/zfs/blob/master/include/os/freebsd/spl/sys/proc.h#L72

Why did you make it __inline? on purpose or by mistake?

do_thread_create was already inline, putting the wrapper in the same place was accident / laziness I suppose

@behlendorf
Copy link
Contributor

@lundman if you can update this to also remove the kthread_exit() from the solthread_wrapper() in the FreeBSD code this should be ready. No need to call it twice now that you've fixed up the consumers.

Undo FreeBSD wrapper for thread_create() added to call thread_exit.

Signed-off-by: Jorgen Lundman <lundman@lundman.net>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks!

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 14, 2020
@behlendorf behlendorf merged commit eeb8fae into openzfs:master May 14, 2020
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Undo FreeBSD wrapper for thread_create() added to call thread_exit.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
Closes openzfs#10314
@lundman lundman deleted the thread_exit branch May 28, 2021 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants