-
Notifications
You must be signed in to change notification settings - Fork 65
Benchmarking #11
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
Benchmarking #11
Conversation
Signed-off-by: Lucy <lucyparker544@gmail.com>
Signed-off-by: Lucy <lucyparker544@gmail.com>
Signed-off-by: Lucy <lucyparker544@gmail.com>
bennoleslie
left a 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.
Overall need to think about how we have benchmarking builds and changes.
Code is very good, no issues there. We just need to think about bigger picture.
I really don't like the idea of requiring different builds for benchmarking vs production.
I think it leads to potential (unintentional) cheating / fooling ourselves by having potential different code running for benchmarks vs production, which I think we should be able to find a good alternative to.
More general comments: with an MR please try to make the MR title a bit longer than just "Benchmarking", and then provide a bit more detail in the MR description.
for example here it could be something like:
"Add support to enable system benchmarking"
and the description would be something like:
Add a benchmark kernel that includes xxxx so that yyyy.
Add TCB cap to PD to enable the code to call xxx so that we get benchmark yyyy.
We should also update the manual appropriately as well.
Let's get the passive server MR complete first, and then rebase this branch off master once the passive server change is merged.
(And apologies, this is my fault for not being able to review in a more timely manner).
| for tcb_obj, schedcontext_obj, pd in zip(tcb_objects, schedcontext_objects, system.protection_domains): | ||
| system_invocations.append(Sel4TcbSetSchedParams(tcb_obj.cap_addr, INIT_TCB_CAP_ADDRESS, pd.priority, pd.priority, schedcontext_obj.cap_addr, fault_ep_endpoint_object.cap_addr)) | ||
|
|
||
| # Copy the PD's TCB cap into their address space for development purposes. |
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 think that this is only needed for development purposes, and following POLA we'd ideally avoid having this cap available in production.
We probably need to think about the best way to support development cap needs vs production cap needs. Could be topic for a Monday meeting discussion. I'll think some more about it.
| "KernelVerificationBuild": False | ||
| } | ||
| ), | ||
| ) |
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.
Let's avoid unrelated changes. In this case the trailing comma was intentional.
What we will do here in future is use: https://black.readthedocs.io/en/stable/index.html to auto-format all the Python code, which should avoid any of these issues.
|
Please review conflicts (ideally rebase from master and push) |
|
Re-done this work in #157 since lots has changed since this PR. |
This PR enables users to specify a 'benchmark' config in order to use seL4's benchmarking system calls.