Rehabilitate _current usage
#83674
Merged
Rehabilitate _current usage
#83674
+529
−561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zephyrbot
requested review from
bbolen,
carlocaione,
ceolin,
cfriedt,
dcpleung,
galak,
ithinuel,
MaureenHelm,
microbuilder,
nashif,
stephanosio and
TaiJuWu
|
Totally slipped out of my mind, thanks for this PR |
andyross
previously approved these changes
Jan 8, 2025
There was a problem hiding this comment.
Tentative +1, with the caveat that I'm going to refuse to engage in any resulting arguments. Not a fan of treewide "naming things" changes in general, especially not of treewide naming things reversions. But I do agree that _current is and was always a cleaner name for the concept (though IIRC I gave a +1 to the rename too...)
added 3 commits
January 8, 2025 13:02
Mostly a revert of commit b1def71 ("arch: deprecate `_current`"). This commit was part of PR zephyrproject-rtos#80716 whose initial purpose was about providing an architecture specific optimization for _current. The actual deprecation was sneaked in later on without proper discussion. The Zephyr core always used _current before and that was fine. It is quite prevalent as well and the alternative is proving rather verbose. Furthermore, as a concept, the "current thread" is not something that is necessarily architecture specific. Therefore the primary abstraction should not carry the arch_ prefix. Hence this revert. Signed-off-by: Nicolas Pitre <[email protected]>
Define the generic _current directly and get rid of the generic arch_current_get(). The SMP default implementation is now known as z_smp_current_get(). It is no longer inlined which saves significant binary size (about 10% for some random test case I checked). Introduce z_current_thread_set() and use it in place of arch_current_thread_set() for updating the current thread pointer given this is not necessarily an architecture specific operation. The architecture specific optimization, when enabled, should only care about its own things and not have to also update the generic _current_cpu->current copy. Signed-off-by: Nicolas Pitre <[email protected]>
Repeated references to _current won't produce a different result as the executing thread instance is always the same. Use the const attribute to let the compiler know it may reuse a previously obtained value. This offset the penalty for moving z_smp_current_get() out of line and provides yet more binary size reduction. This change is isolated in its own commit to ease bisecting in case some unexpected misbehavior is eventually observed. Signed-off-by: Nicolas Pitre <[email protected]>
andyross
reviewed
Jan 8, 2025
|
@andyross wrote:
Refresh +1
Forgot to push the "approve" button?
|
nashif
approved these changes
Jan 8, 2025
andyross
approved these changes
Jan 8, 2025
cfriedt
approved these changes
Jan 9, 2025
ycsin
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the revert of the (unwarranted IMHO)
_currentdeprecation asI suggested in PR #80716. Additional cleanups are also included.