Fix stack switching on OpenCilk's runtime#363
Open
kennyzzhang wants to merge 5 commits into
Open
Conversation
Signed-off-by: kennyzzhang <kennyzzhang@gmail.com>
longjmp_to_user_code pushes to inactive stacks, longjmp_to_runtime pops from inactive stacks Signed-off-by: kennyzzhang <kennyzzhang@gmail.com>
Signed-off-by: kennyzzhang <kennyzzhang@gmail.com>
Signed-off-by: kennyzzhang <kennyzzhang@gmail.com>
Signed-off-by: kennyzzhang <kennyzzhang@gmail.com>
Xyene
reviewed
May 5, 2026
Xyene
left a comment
Xyene
left a comment
Member
There was a problem hiding this comment.
Thanks, looks mostly good! A couple of nits/suggestions.
| let ret = ret_without_checking_for_go_hacks | ||
|
|
||
| let switch_to_user_code t (thread_info : _ Thread_info.t) ~time = | ||
| (* Pop the sysdep_longjmp_to_sf frame *) |
Member
There was a problem hiding this comment.
Suggested change
| (* Pop the sysdep_longjmp_to_sf frame *) | |
| (* Pop the [sysdep_longjmp_to_sf] frame. *) |
| between user and runtime stacks. | ||
|
|
||
| To deal with this stack switching, when jumping into the runtime, we need to clear the | ||
| user's stack frames. The inactive_callstacks mechanism is perfect for keeping track of |
Member
There was a problem hiding this comment.
Suggested change
| user's stack frames. The inactive_callstacks mechanism is perfect for keeping track of | |
| user's stack frames. The [inactive_callstacks] mechanism is perfect for keeping track of |
| let switch_to_user_code t (thread_info : _ Thread_info.t) ~time = | ||
| (* Pop the sysdep_longjmp_to_sf frame *) | ||
| ret t thread_info ~time; | ||
| (* The next stack frame is either __cilkrts_sync or longjmp_to_user_code. In either |
Member
There was a problem hiding this comment.
Suggested change
| (* The next stack frame is either __cilkrts_sync or longjmp_to_user_code. In either | |
| (* The next stack frame is either [__cilkrts_sync] or [longjmp_to_user_code]. In either |
| thread_info.callstack <- Callstack.create ~create_time:time | ||
| | "__cilkrts_sync" -> ret t thread_info ~time | ||
| | _ -> Printf.printf "OpenCilk_hacks: Unexpected symbol %s\n" symbol) | ||
| | _ -> Printf.printf "OpenCilk_hacks: Unexpected symbol [unknown]" |
Member
There was a problem hiding this comment.
Nit: these are not just [unknown] symbols. Writing the match exhaustively would make that a little more clear.
| Stack.push thread_info.inactive_callstacks thread_info.callstack; | ||
| thread_info.callstack <- Callstack.create ~create_time:time | ||
| | "__cilkrts_sync" -> ret t thread_info ~time | ||
| | _ -> Printf.printf "OpenCilk_hacks: Unexpected symbol %s\n" symbol) |
Member
There was a problem hiding this comment.
Nit: let's use eprintf here, and use the %! syntax to force a flush. For example, see
| Thread_info.set_callstack thread_info ~is_kernel_address ~time | ||
| ;; | ||
|
|
||
| (* OpenCilk's runtime cheetah (https://github.com/OpenCilk/cheetah) uses longjmp to switch |
Member
There was a problem hiding this comment.
Nit: let's move all of this code up, so that let call ... = ... is close in the file with let ret ... = ....
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
OpenCilk's work stealing scheduler cheetah uses longjmp to switch between user and runtime code. This causes the stackframes to stairstep downwards, as expected from the FAQ.
So, I've "added some explicit code in magic-trace for [OpenCilk]".
In particular, upon jumping to user code, the runtime's stack is put into an inactive stack, which is then restored when jumping back to the user code.
The main two functions in cheetah that call longjmp are
longjmp_to_runtimeandsysdep_longjmp_to_sf.sysdep_longjmp_to_sfis called from two different places,longjmp_to_user_codeor__cilkrts_sync.This means an extra frame needs to be popped when
sysdep_longjmp_to_sfis called, and we should only put the runtime stack into an inactive stack when we seelongjmp_to_user_code.This is my first time writing OCaml, and honestly I was fumbling around copying and adapting other parts of the codebase to coerce things to work. But it seems like things work well enough that it's now much easier to tell if Cilk workers are failing to find work.