Skip to content

Conversation

@lucypa
Copy link
Contributor

@lucypa lucypa commented Aug 19, 2022

This PR enables users to specify a 'benchmark' config in order to use seL4's benchmarking system calls.

lucypa added 3 commits August 15, 2022 13:55
Signed-off-by: Lucy <lucyparker544@gmail.com>
Signed-off-by: Lucy <lucyparker544@gmail.com>
Signed-off-by: Lucy <lucyparker544@gmail.com>
Copy link
Contributor

@bennoleslie bennoleslie left a 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.
Copy link
Contributor

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
}
),
)
Copy link
Contributor

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.

@bennoleslie
Copy link
Contributor

Please review conflicts (ideally rebase from master and push)

@Ivan-Velickovic
Copy link
Collaborator

Re-done this work in #157 since lots has changed since this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants