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.
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
SIMD-0182: Consume requested CUs for sBPF failures #182
base: main
Are you sure you want to change the base?
SIMD-0182: Consume requested CUs for sBPF failures #182
Changes from all commits
55e4924
989a88f
6f6e049
19a48f1
08a9f56
9a22dfc
d307391
30f088c
d439937
a69ec93
0aed128
0dfaa4d
7350797
5194227
e8c74ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 see that with direct mapping enabled, we convert
EbpfError::AccessViolation
into theSyscallError
variant here: https://github.com/anza-xyz/agave/blob/af0ed22174999cb62579a0621f6b274c85ebf267/programs/bpf_loader/src/lib.rs#L1501I am assuming that this access violation should also deplete compute units and should not be considered as an exception right?
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.
@Lichtso @alessandrod
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.
Yes, same reasoning as: #182 (comment)
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.
Should consume all cu's or not?
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.
Yes, we should. This is not caused by a syscall so the exact instruction counter would have to be calculated otherwise.
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.
Just want to call out that this type of access violation is probably going to be fairly common when direct mapping is enabled. So it's not exactly true to say that these types of errors are "rare, exceptional situations" or "A rare case" as we say in the SIMD. We should update the SIMD to say that these types of failures are "less common" rather than rare at least.
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.
e8c74ac
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.
@tao-stones this should probably be made explicit in the SIMD. Maybe we can say that all EBPF errors from the vm are treated as irregular.
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.
Mmm, imo, SIMD's text
If VM execution returns any error except SyscallError, ....
is clearer.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.
Just out of curiosity, why make syscall errors exempt?
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.
My understanding is syscall not happen in VM realm. @Lichtso @ptaffet-jump please correct me if I'm wrong.
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.
Syscall errors are never thrown in the middle of basic blocks, so we can easily know exactly how many CUs were consumed when the error was thrown. This is why we can safely make them exempt.