Hi Yipeng,
> -----Original Message-----
> From: Wang, Yipeng1
> Sent: Friday, June 8, 2018 11:51 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>
> Cc: dev@dpdk.org; Wang, Yipeng1 <yipeng1.w...@intel.com>; Mcnamara,
> John <john.mcnam...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; honnappa.nagaraha...@arm.com;
> vgu...@caviumnetworks.com; brijesh.s.si...@gmail.com
> Subject: [PATCH v1 2/3] test: add test case for read write concurrency
>
> This commit adds a new test case for testing read/write concurrency.
Could you split this patch into two? One adding lock "support" in the current
performance test code and another one adding the new readwrite tests?
>
> Signed-off-by: Yipeng Wang <yipeng1.w...@intel.com>
> ---
> test/test/Makefile | 1 +
> test/test/test_hash_perf.c | 36 ++-
> test/test/test_hash_readwrite.c | 649
...
> +++ b/test/test/test_hash_readwrite.c
...
> +struct {
> + uint32_t *keys;
> + uint32_t *found;
> + uint32_t nb_tsx_insertion;
> + uint32_t rounded_nb_total_tsx_insertion;
> + struct rte_hash *h;
> +} tbl_multiwriter_test_params;
I think " rounded_nb_total_tsx_insertion" and " tbl_multiwriter_test_params"
are too long, and that's why you need to split a long line into two down below.
Could you shorten these names a bit? You can change "multiwriter" to "mw",
and "rounded_nb_total_tsx_insertion" to "total_nb_tsx_inserts".
...
> + snprintf(name, 32, "tests");
> + hash_params.name = name;
You can set hash_params.name = "tests" directly.
> +
> + handle = rte_hash_create(&hash_params);
> + if (handle == NULL) {
> + printf("hash creation failed");
> + return -1;
> + }
...
> +
> +err2:
> + rte_free(keys);
> +err1:
> + rte_hash_free(handle);
I think you can have just one label "err" with these two frees.
If the variables haven't been set, they will be NULL and that's allowed.
> +
> + return -1;
> +}
> +
...
> + begin = rte_rdtsc_precise();
> + for (i = 0; i < read_cnt; i++) {
> + void *data;
> + rte_hash_lookup_data(tbl_multiwriter_test_params.h,
> + tbl_multiwriter_test_params.keys + i,
> + &data);
> + if (i != (uint64_t)data) {
I see the following error and other errors when compiling with gcc 32 bits.
test/test/test_hash_readwrite.c:281:12: error:
cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
if (i != (uint64_t)data) {
^
> + printf("lookup find wrong value %d, %ld\n", i,
> + (uint64_t)data);
...
> +
> + if (reader_faster) {
> + unsigned long long int cycles_per_insertion =
> + rte_atomic64_read(&gread_cycles)/
> + rte_atomic64_read(&greads);
> + perf_results->read_only[n] = cycles_per_insertion;
> + printf("Reader only: cycles per lookup: %llu\n",
> + cycles_per_insertion);
> + }
> +
> + else {
} else {
...
> + use_htm = 1;
> + if (test_hash_readwrite_functional() < 0)
> + return -1;
> +
> + reader_faster = 1;
Maybe a comment about this reader_faster would be good.
Also, can we declare this variable and use_html as local and pass them to the
functions,
instead of declaring them as global?
> + if (test_hash_readwrite_perf(&htm_results) < 0)
> + return -1;
