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

[Cyril Hrubis <chrubis@suse.cz>]

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)


>  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
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp