-
Notifications
You must be signed in to change notification settings - Fork 28
Add concurrency checking based on GenMC #523
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
Draft
KurtWu10
wants to merge
10
commits into
main
Choose a base branch
from
genmc
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
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
Signed-off-by: Kurt Wu <rihui.wu@unsw.edu.au>
Signed-off-by: Kurt Wu <rihui.wu@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Collaborator
|
Made some minor fixes to make CI pass. |
move benchmark config macro to the header file referenced by both benchmark.c and idle.c Signed-off-by: Terry Bai <terry.z.bai@gmail.com>
Signed-off-by: Kurt Wu <rihui.wu@unsw.edu.au>
Signed-off-by: Kurt Wu <rihui.wu@unsw.edu.au>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduces concurrency testing of the single-producer single-consumer queue in the network subsystem using the GenMC model checker. This enables lightweight and reliable CI testing of the queue's concurrency. To pass GenMC's verification, this PR also refactors the queue implementation.
Testing is performed using a standard multi-threaded C program (
ci/genmc/network/test1.c) that uses the queue and embeds functional correctness requirements. This allows the same program to be simulated by the GenMC tool or compiled and run on real hardware without significant modification.In the current test program, the queue is tested using busy loops, because the test only consider partial correctness of the queue. The test also does not consider deadlock-freedom of the queue. The test only covers the free queue currently, but not the active queue. Interactions with the signaling protocol are not considered in the PR.
Tests for queues in other subsystems will be added in future PRs.
The following changes were made to the queue implementation to pass GenMC's checks:
Certain accesses are explicitly labelled as atomic accesses, following a similar implementation in LionsOS. These atomic accesses use the release-acquire subset of the C memory model. Each access is annotated to indicate which other atomic access it synchronises with.
The distinction of whether the queue will be run on SMP Microkit is removed, following sddf queue functional correctness #511 (comment).In a later revision of this PR, the distinction is retained due to the overhead from
stlr(~8% on cpu utilisation on Maaxboard under full load). Compiler fences, whose semantic are similar to the pancake implementation, have been introduced explicitly.The
net_queue_lengthfunction is removed because It is not used.Functional correctness of the modified queue does not include synchronisations induced by the queue.
These changes may have both functional correctness and performance implications that should be addressed before merging:
ldarinstruction instead of thedmb ishinstruction on existing platforms. Since the former is usually weaker than the latter in the Arm memory model, it might reveal under-synchronisation issues in the system, because the GenMC testing only cover correctness of the queue itself.especially under the non-SMP setting, the atomic instruction may introduce overheadthere should be no performance overhead from the current implementation under the non-SMP setting. Under the SMP setting, it is claimed that thedmb stbarrier is faster thanstlrfor store-store ordering, which should be validated on our platforms.Finally, under the non-SMP pancake configuration,
the proposed solution introduces discrepancy in the queue implementationthe latest proposed solution remains implementable and existing code does not need to be modified. The proposed solution does not change the number of FFIs required under the SMP pancake configuration.