[LTP] [PATCH 1/2] Enhanced thread safety in ebizzy benchmark tool

[Vishal Chourasia]

On 8/14/23 20:45, Cyril Hrubis wrote:
> Hi!
>> Modified ebizzy.c to improve thread safety by introducing a mutex for
>> 'records_read' shared variable.
>>
>> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Reviewed-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>> ---
>>  utils/benchmark/ebizzy-0.3/ebizzy.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/utils/benchmark/ebizzy-0.3/ebizzy.c b/utils/benchmark/ebizzy-0.3/ebizzy.c
>> index 54b047130..1af004d9d 100644
>> --- a/utils/benchmark/ebizzy-0.3/ebizzy.c
>> +++ b/utils/benchmark/ebizzy-0.3/ebizzy.c
>> @@ -85,6 +85,8 @@ static time_t start_time;
>>  static volatile int threads_go;
>>  static unsigned int records_read;
>>  
>> +pthread_mutex_t records_read_lock;
> 
> Can't we just initialze the mutex statically?
> 
> i.e.:
> 
> static pthread_mutex_t record_read_lock =  PTHREAD_MUTEX_INITIALIZER;
> 
> 
> Then we don't have to intialize/destroy it.>
> Or even better we can rework the code so that the value is passed up to
> the pthread_join() function which would serialize the code naturally,
> something as:
> 
> diff --git a/utils/benchmark/ebizzy-0.3/ebizzy.c b/utils/benchmark/ebizzy-0.3/ebizzy.c
> index 54b047130..841bf0a1c 100644
> --- a/utils/benchmark/ebizzy-0.3/ebizzy.c
> +++ b/utils/benchmark/ebizzy-0.3/ebizzy.c
> @@ -50,6 +50,7 @@
>  #include <time.h>
>  #include <sys/time.h>
>  #include <sys/resource.h>
> +#include <stdint.h>
> 
>  #include "ebizzy.h"
> 
> @@ -83,7 +84,6 @@ static char **hole_mem;
>  static unsigned int page_size;
>  static time_t start_time;
>  static volatile int threads_go;
> -static unsigned int records_read;
> 
>  static void usage(void)
>  {
> @@ -423,6 +423,8 @@ static unsigned int search_mem(void)
> 
>  static void *thread_run(void *arg __attribute__((unused)))
>  {
> +       uintptr_t records_thread;
> +
>         if (verbose > 1)
>                 printf("Thread started\n");
> 
> @@ -430,13 +432,13 @@ static void *thread_run(void *arg __attribute__((unused)))
> 
>         while (threads_go == 0) ;
> 
> -       records_read += search_mem();
> +       records_thread = search_mem();
> 
>         if (verbose > 1)
>                 printf("Thread finished, %f seconds\n",
>                        difftime(time(NULL), start_time));
> 
> -       return NULL;
> +       return (void *)records_thread;
>  }
> 
>  static struct timeval difftimeval(struct timeval *end, struct timeval *start)
> @@ -454,6 +456,7 @@ static void start_threads(void)
>         unsigned int i;
>         struct rusage start_ru, end_ru;
>         struct timeval usr_time, sys_time;
> +       unsigned int records_read = 0;
>         int err;
> 
>         if (verbose)
> @@ -484,11 +487,13 @@ static void start_threads(void)
>          */
> 
>         for (i = 0; i < threads; i++) {
> -               err = pthread_join(thread_array[i], NULL);
> +               uintptr_t record_thread;
> +               err = pthread_join(thread_array[i], (void *)&record_thread);
>                 if (err) {
>                         fprintf(stderr, "Error joining thread %d\n", i);
>                         exit(1);
>                 }
> +               records_read += record_thread;
>         }
> 
>         if (verbose)
> 
> 
Thank you for your review and insightful suggestions. Approach of
leveraging pthread_join() to naturally serialize the code and retrieve
the records_thread value is certainly elegant. By doing so, we can avoid
the overhead of mutex initialization and destruction, making the code
cleaner and readable.

I appreciate your expertise and input on this matter. I'll incorporate
your suggestions and send out the next version of the patch soon.

vishalc
>>  static void usage(void)
>>  {
>>  	fprintf(stderr, "Usage: %s [options]\n"
>> @@ -430,7 +432,9 @@ static void *thread_run(void *arg __attribute__((unused)))
>>  
>>  	while (threads_go == 0) ;
>>  
>> +	pthread_mutex_lock(&records_read_lock);
>>  	records_read += search_mem();
>> +	pthread_mutex_unlock(&records_read_lock);
>>  
>>  	if (verbose > 1)
>>  		printf("Thread finished, %f seconds\n",
>> @@ -456,6 +460,12 @@ static void start_threads(void)
>>  	struct timeval usr_time, sys_time;
>>  	int err;
>>  
>> +	/* Initialize the mutex before starting the threads */
>> +	if (pthread_mutex_init(&records_read_lock, NULL) != 0) {
>> +		fprintf(stderr, "Failed to initialize mutex\n");
>> +		exit(1);
>> +	}
>> +
>>  	if (verbose)
>>  		printf("Threads starting\n");
>>  
>> @@ -491,6 +501,8 @@ static void start_threads(void)
>>  		}
>>  	}
>>  
>> +	pthread_mutex_destroy(&records_read_lock);
>> +
>>  	if (verbose)
>>  		printf("Threads finished\n");
>>  
>> -- 
>> 2.39.3
>>
>