Lockhammer -o option colon separator instead of comma #36
Conversation
Add new thread pinning order option -o and lstopo parser to sweep test
Sync with upstream for lstopo feature
Because we use comma separated value file format for the output, it's better to use colon instead of comma to separate -o arguments. We also split lh_test_cfg.yaml into two config file: sweeptest and unittest, because these two tests normally don't run together. We also modified runall.sh to use test_lockhammer.py instead of plain bash script.
|
Can one of the admins verify this patch? |
geoffreyblake
left a comment
geoffreyblake
left a comment
There was a problem hiding this comment.
I am not sure what these cleanups are really accomplishing.
| return 1; | ||
| } | ||
| csv = strtok(optarg, ","); | ||
| /* colon is better than comma because lockhammer output uses csv format */ |
There was a problem hiding this comment.
I don't see the issue here. Using a comma to delimit a list is more natural.
There was a problem hiding this comment.
The output result file is like this if we keep using comma instead of colon:
1, 0.659050, 27.533800, 27.936200, 41.778000, 0.000000, 2018-06-21 00:51:06.954413, epyc7601, ../build/lh_cas_event_mutex, -t, 1, -a, 5000, -c, 0ns, -p, 0ns, -o, 0,32,8,40,16,48,24,56,4,36,12,44,20,52,28,60,1,33,9,41,17,49,25,57,5,37,13,45,21,53,29,61,2,34,10,42,18,50,26,58,6,38,14,46,22,54,30,62,3,35,11,43,19,51,27,59,7,39,15,47,23,55,31,63
For python csv file loader, it is a little confusing.
If we change -o comma to colon, it's much easier for the python result parser to load:
1, 0.659050, 27.533800, 27.936200, 41.778000, 0.000000, 2018-06-21 00:51:06.954413, epyc7601, ../build/lh_cas_event_mutex, -t, 1, -a, 5000, -c, 0ns, -p, 0ns, -o, 0:32:8:40:16:48:24:56:4:36:12:44:20:52:28:60:1:33:9:41:17:49:25:57:5:37:13:45:21:53:29:61:2:34:10:42:18:50:26:58:6:38:14:46:22:54:30:62:3:35:11:43:19:51:27:59:7:39:15:47:23:55:31:63
The colon sign can also be perceived as POSIX path/way separator semantically.
There was a problem hiding this comment.
I agree with both arguments: commas on the command line make for more sensible list separation but colons make parsing the result easier since we don't have to distinguish between commas that separate fields and comas that separate list entries. Is it possible to have the output always dump colons but the input accept either colons or commas?
There was a problem hiding this comment.
OK, but accepting both colon and comma will make option parsing code more complex:
- It needs to detect if it is colon or comma separated, or perhaps both.
- And output result may not reflect the actual input parameters.
- Help messages will be messy as well.
I think colon only should be ok, especially at this early stage, strict syntax is better because there is no history burden.
There was a problem hiding this comment.
This can be achieved by simply:
csv = strtok(optarg, ":,");
To allow either ':' or ',' (or any combination thereof within the argument) as a tokenizing character.
In such a case output arguments should always reflect a input option which is functionally identical to whatever input was provided. Help messages can prefer one of the options and the code can silently allow the other.
There was a problem hiding this comment.
Done, support both comma and colon and help message only shows colon as official delimiter for -o option. Fixed and verified on arm64 system in latest patch.
Let help be more readable...
|
Verified on EPYC server. |
Default output will still use comma as delimiter like csv file. test_lockhammer.py will generate colon as delimiter in default runall.sh
|
Also verified on latest arm64 ubuntu system. |
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, ARM-software#4, ARM-software#36, ARM-software#8 individually. Processor #0 and ARM-software#32 are in the same physical core. Processor ARM-software#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.
3 participants
Because we also output csv comma separated file as result, it's better to use another operator like colon instead of comma for -o parameter separation.
We also made small modification on sweeptest and unittest and split config file into two.