Re: [Qemu-devel] [PATCH v6 12/15] qht: add qht-bench, a performance benchmark

[Sergey Fedorov]

On 03/06/16 14:41, Emilio G. Cota wrote:
> On Sun, May 29, 2016 at 23:45:23 +0300, Sergey Fedorov wrote:
>> On 25/05/16 04:13, Emilio G. Cota wrote:
>>> diff --git a/tests/qht-bench.c b/tests/qht-bench.c
>>> new file mode 100644
>>> index 0000000..30d27c8
>>> --- /dev/null
>>> +++ b/tests/qht-bench.c
>>> @@ -0,0 +1,474 @@
>> (snip)
>>> +static void do_rw(struct thread_info *info)
>>> +{
>>> +    struct thread_stats *stats = &info->stats;
>>> +    uint32_t hash;
>>> +    long *p;
>>> +
>>> +    if (info->r >= update_threshold) {
>>> +        bool read;
>>> +
>>> +        p = &keys[info->r & (lookup_range - 1)];
>>> +        hash = h(*p);
>>> +        read = qht_lookup(&ht, is_equal, p, hash);
>>> +        if (read) {
>>> +            stats->rd++;
>>> +        } else {
>>> +            stats->not_rd++;
>>> +        }
>>> +    } else {
>>> +        p = &keys[info->r & (update_range - 1)];
>>> +        hash = h(*p);
>> The previous two lines are common for the both "if" branches. Lets move
>> it above the "if".
> Not quite. The mask uses lookup_range above, and update_range below.

Ah, yes :)

>
>>> +
>>> +    rcu_read_lock();
>> Why don't we do rcu_read_lock()/rcu_read_unlock() inside the loop?
> Because that will slow down the benchmark unnecessarily (Throughput
> for single-threaded and default opts goes down from 38M/s to 35M/s).
> For this benchmark we want to benchmark QHT's performance, not RCU's.
> And really we're not allocating/deallocating elements dynamically,
> so from a memory usage viewpoint calling this inside or outside
> of the loop doesn't matter.

Fair enough.

>
>>> +    while (!atomic_read(&test_stop)) {
>>> +        info->r = xorshift64star(info->r);
>>> +        info->func(info);
>>> +    }
>>> +    rcu_read_unlock();
>>> +
>>> +    rcu_unregister_thread();
>>> +    return NULL;
>>> +}
>> (snip)
>>> +static void run_test(void)
>>> +{
>>> +    unsigned int remaining;
>>> +    int i;
>>> +
>> Are we sure all the threads are ready at this point? Otherwise why
>> bother with 'test_start' flag?
> Good point. Added the following:
>
> diff --git a/tests/qht-bench.c b/tests/qht-bench.c
> index 885da9c..c1ed9b9 100644
> --- a/tests/qht-bench.c
> +++ b/tests/qht-bench.c
> @@ -43,6 +43,7 @@ static unsigned long lookup_range = DEFAULT_RANGE;
>  static unsigned long update_range = DEFAULT_RANGE;
>  static size_t init_range = DEFAULT_RANGE;
>  static size_t init_size = DEFAULT_RANGE;
> +static size_t n_ready_threads;
>  static long populate_offset;
>  static long *keys;
>  
> @@ -190,6 +191,7 @@ static void *thread_func(void *p)
>  
>      rcu_register_thread();
>  
> +    atomic_inc(&n_ready_threads);
>      while (!atomic_mb_read(&test_start)) {
>          cpu_relax();
>      }
> @@ -387,6 +389,9 @@ static void run_test(void)
>      unsigned int remaining;
>      int i;
>  
> +    while (atomic_read(&n_ready_threads) != n_rw_threads + n_rz_threads) {
> +        cpu_relax();
> +    }
>      atomic_mb_set(&test_start, true);
>      do {
>          remaining = sleep(duration);
>

Looks good.

Kind regards,
Sergey