-
Notifications
You must be signed in to change notification settings - Fork 234
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
Implement implicit_context
for helper functions.
#3656
Merged
saxena-anurag
merged 46 commits into
microsoft:main
from
saxena-anurag:user/anusa/implicit_ctx
Jul 19, 2024
Merged
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
5b21e46
backup
saxena-anurag f2812bc
backup
saxena-anurag 40346b3
fix build issues
saxena-anurag a14a07e
fix build
saxena-anurag 66e812c
fix analysis failures, fix tests
saxena-anurag 6a14c3f
seperate out sample for implicit context
saxena-anurag c1aab90
backup
saxena-anurag d2c2b0e
add ctx as last argument
saxena-anurag f072525
fix hash
saxena-anurag 3a728ef
block implicit context for jit and interpret
saxena-anurag 2780c14
stabilize tests
saxena-anurag cd690e6
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 53648a6
tail call fast path
saxena-anurag aa29fd9
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 6d1fddb
add check to reject change in context_header support
saxena-anurag 87547a8
enable implicit context for jit and interpret
saxena-anurag d92316e
fix
saxena-anurag 26bfaf0
fix build
saxena-anurag 6e251f7
fix analysis build
saxena-anurag aeaa633
update version, update expected files
saxena-anurag 0fdce04
fix test failures
saxena-anurag 33cc42f
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 7411c6b
fix test failures
saxena-anurag 1ea7026
fix tests
saxena-anurag dc71fc2
cleanup
saxena-anurag 1bdb594
cleanup
saxena-anurag ce6a01b
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag a5a2e33
more cleanup
saxena-anurag 84e97ab
enable performance tests
saxena-anurag 46e4e7b
enable performance tests
saxena-anurag f96510d
remove tail_call changes
saxena-anurag 7e99371
remove tail_call changes
saxena-anurag 2f0dbf3
remove tail_call changes
saxena-anurag c940bbb
remove tail_call changes
saxena-anurag 289e5d8
fix analysis error
saxena-anurag 202f72e
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 0272efa
update documentation
saxena-anurag 88784c8
code cleanup
saxena-anurag 0b30763
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 695d62f
fix bad merge
saxena-anurag 86b15c1
fix test case
saxena-anurag dc60a09
cr comments
saxena-anurag 6a4e3c4
update expected files
saxena-anurag 0fb7b8d
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag 9b65b9d
Merge branch 'main' into user/anusa/implicit_ctx
saxena-anurag f119d0b
cr comments
saxena-anurag File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:
implicit_context
is added as a separate bool instead of adding a bit field in existingebpf_helper_function_prototype_flags_t
flags to handle forward compatibility issues.Details:
The
implicit_context
change in this PR is optional and backward compatible but is not forward compatible. This means that if an extension is compiled with an older (<= 0.17.x) version of eBPF headers and is deployed on a machine running a newer (>= 0.18.x) version of eBPF runtime, it can safely run with no issues.On the other hand, if the extension is compiled with a newer version of eBPF and supports implicit context for any of its helper functions, and gets deployed with older eBPF runtime, it can cause a crash unless eBPF runtime rejects binding with such an extension.
Adding a new
implicit_context
field inebpf_helper_function_prototype_t
changes the size of the struct allowing eBPF runtime o detect and reject incompatible version of extension.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with blocking this, but I am not sure overloading the verification check as being the correct approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-opening as I have a question about this - In the future, when would it be appropriate to add to the flags field vs requiring a new bool?
Do we maybe want to add documentation to help add clarity for future developers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think if the new feature is both backward and forward compatible, it can be added to the existing flags. Agree that we can probably document this somewhere, but not sure which doc (ebpfExtension.md is ideally for extension developers, but this will be for eBPF developers). I can maybe add a comment in this file itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated doc to mention this point.