Lockhammer: improve command line arg parsing #8
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
Merge in lockhammer benchmark from integration branch. Signed-off-by: Geoffrey Blake (Geoffrey.Blake@arm.com)
Convert argument parsing from only default/full specification to incremental flags and parameters. Additionally allow for test- specific arguments to be provided on the command line following "--". Fixes ARM-software#7 Change-Id: Ia97684b6b6df72f2fb8f057a73182aae31f6276f Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
Move argument types to a header file in part to clean up main source file. This change also exposes the argument containing struct to the test so that the test-specific argument parser can utilize the values of the generic command line arguments. Change-Id: I2fb62e5afa18ccb85c7ee95d8803edf823a1d02f Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
Default (invalid arg) case now exits in addition to printing usage message. Change-Id: I73da89c7bcb0a3b4ee2a70404efdfb17616f4434 Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
geoffreyblake
suggested changes
Apr 30, 2018
Contributor
geoffreyblake
left a comment
geoffreyblake
left a comment
There was a problem hiding this comment.
Parsing should be done with a standard library function like getopt in the C library.
|
|
||
| void print_usage (char *invoc) { | ||
| fprintf(stderr, | ||
| "Usage: %s\n\t[-t threads]\n\t[-a acquires per thread]\n\t" \ |
Contributor
There was a problem hiding this comment.
This can be indented as normal, and the trailing \ is not needed for strings to be continued.
| if (errno == EINVAL) { | ||
| print_usage(argv[0]); | ||
| return 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of writing our own command line parser, why not use getopt? When getopt hits a "--" it will stop parsing and we can then parse the remainder if needed in your optional parse_test_args function per test.
Use getopt for more robust argument parsing instead of reimplementing. Change-Id: If65277243d5ac32b71b9d8757dd3516673c184a7 Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
geoffreyblake
suggested changes
May 4, 2018
Contributor
geoffreyblake
left a comment
geoffreyblake
left a comment
There was a problem hiding this comment.
Please see the comments below.
| fprintf(stderr, | ||
| "Usage: %s\n\t[-t threads]\n\t[-a acquires per thread]\n\t" | ||
| "[-c critical iterations]\n\t[-p parallelizable iterations]\n\t" | ||
| "[-- <test specific arguments>]\n", invoc); |
Contributor
There was a problem hiding this comment.
I guess we're dropping this for now? I do not see it handled in the getopt code.
| args.nthrds > num_cores ? num_cores : args.nthrds; | ||
| break; | ||
| case 'a': | ||
| args.nacqrs = strtol(optarg, (char **) NULL, 10); |
Contributor
There was a problem hiding this comment.
I highly advise that the inputs are sanitized here. We are using strtol() here, which can accept negative numberes, and then casting them to unsigned ints which could lead to unpredictable behavior.
| #define __LOCKHAMMER_H__ | ||
|
|
||
| struct thread_args { | ||
| unsigned long ncores; |
Contributor
There was a problem hiding this comment.
Should turn these into signed variables for the input parameters from the command line to check for invalid input, like negative numbers.
added 2 commits
May 7, 2018 16:30
Because the hybrid spinlock requires LL/SC support in order to avoid touching the ticket value if queueing has already begun it cannot be implemented on x86_64 as designed. Only build the two hybrid spinlock tests if the target arch of the compiler is aarch64. Fixes ARM-software#11 Change-Id: I33c79dbe1a81bc978f2029b2c13a902244415663 Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
Refuse to run if any command-line arguments are negative. Issue a warning if requested thread count exceeds online core count but contiue to run with thread count limited to online core count. Change-Id: I36a02e0909e851f8c5c6483b953d0232eab37679 Signed-off-by: Lucas Crowthers <lucasc.qdt@qualcommdatacenter.com>
geoffreyblake
approved these changes
May 8, 2018
zoybai
pushed a commit
to zoybai/synchronization-benchmarks
that referenced
this pull request
Jun 19, 2018
Because some platforms may have drastically different core-thread topology, especially for multi-socket NUMA scenario. We add another -o option to specify the preferred "order" for sweep tests. This parameter consists of individual core numbers from 0 to the max core number, separated by comma without space. With this option, lockhammer can pin its threads according to this arbitrarily provided order without any complex calculation. Then we can easily change the order in test_lockhammer.py without modifying lockhammer.c source code. Currently, we deduce the order from this command by extracting P#: lstopo --no-io --no-caches |grep PU For example, if we want to test 5 threads on 32-core single-socket EPYC server, we may use the following pinning order: ./build/lh_osq_lock -t 5 -a 10000000 -o 0,32,4,36,8,40,12,44,16,48, 20,52,24,56,28,60,1,33,5,37,9,41,13,45,17,49,21,53,25,57,29,61,2,34, 6,38,10,42,14,46,18,50,22,54,26,58,30,62,3,35,7,39,11,43,15,47,19, 51,23,55,27,59,31,63 Because there are only 5 threads, lockhammer will pin each thread to processor #0, ARM-software#32, #4, ARM-software#36, ARM-software#8 individually. Processor #0 and ARM-software#32 are in the same physical core. Processor #4 and ARM-software#36 are in another same physical core. Processor ARM-software#8 are in a different core compared to the first 4 logical processors. Therefore we removed the necessity of introducing yet another interleaving mode to lockhammer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Convert argument parsing from only default/full specification to
incremental flags and parameters. Additionally allow for test-
specific arguments to be provided on the command line following
"--".
Fixes #7
Change-Id: Ia97684b6b6df72f2fb8f057a73182aae31f6276f
Signed-off-by: Lucas Crowthers lucasc.qdt@qualcommdatacenter.com