i#7157 dyn inject: Fix extra switch injections#7302
Merged
abhinav92003 merged 6 commits intomasterfrom Feb 25, 2025
Merged
Conversation
Fixes a bug in the dynamic injection logic in the drmemtrace scheduler that caused the context switch sequence to be injected more times than actual context switches. It was erroneously injected also when set_cur_input was invoked without any change to the input. update_switch_stats() is a better control point for switch sequence injection, which is renamed to on_context_switch here. Now, in the switch insertion offline test, we have 11 context switch injections (== input-to-input + idle-to-input switches), instead of the prior 363 Issue: #7157
This was referenced Feb 25, 2025
abhinav92003
added a commit
that referenced
this pull request
Feb 25, 2025
Adds the count of instances where the context switch sequence was injected to the schedule_stats tool output. Modifies the switch_insertion test to use schedule_stats instead of basic_counts. This helps reveal a bug in context switch trace template dynamic injection logic that causes the injection to happen many more times than the context switch events. This diff is split out from #7302 which fixes the above bug. Issue: #7157
derekbruening
approved these changes
Feb 25, 2025
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.
Fixes a bug in the dynamic injection logic in the drmemtrace scheduler that caused the context switch sequence to be injected more times than actual context switches. It was erroneously injected also when set_cur_input was invoked without any change to the input.
update_switch_stats() is a better control point for switch sequence injection, which is renamed to on_context_switch() here. Now, in the switch_insertion offline test, we have 11 context switch injections (== input-to-input + idle-to-input switches), instead of the prior 363.
We deliberately do not inject the context switch sequence on input-to-idle transitions. Note that we already inject on idle-to-input transitions, and it is not clear yet whether we want to repeat it at input-to-idle also or maybe inject some different sequence.
The above move also removes the use of get_instruction_ordinal() to determine whether switch records should be inserted. Existing logic checked for get_instruction_ordinal() > 0 to avoid inserting switch records before the very first input on an output. This does not work as required when USE_INPUT_ORDINALS is set, as then the input-local instruction ordinal instead (which is non-zero as it is not adjusted for the read-ahead instructions) is returned, which ends up inserting the switch sequence before the very first input also. A scheduler unit_test that uses USE_INPUT_ORDINALS is coming up in #7299 with more fixes.
Issue: #7157