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

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

[Vishal Chourasia]

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


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

[LTP] [PATCH 0/2] Ebizzy Benchmark Tool: Enhancements for Thread Safety and Record Count Handling

[Vishal Chourasia]

Patch 1: Enhancing Thread Safety in Ebizzy Benchmark Tool

This patch is aimed at improving the thread safety in the ebizzy benchmarking
tool. A potential issue was identified where the shared variable 'records_read'
was susceptible to race conditions when concurrently updated by multiple
threads. This behavior could lead to skewing of the benchmark results. To
mitigate this issue, a mutex for 'records_read' is introduced in this patch. The
mutex ensures thread-safe updates and consequently improves the reliability of
the benchmark. 

Patch 2: Enhanced Record Count Handling

This patch modifies `ebizzy.c` to handle situations with high record counts more
effectively, especially when the count exceeds `UINT_MAX`. It includes adjusting
the `records_read` variable type from `unsigned int` to `unsigned long
long`. This adjustment allows the benchmark tool to handle a broader range of
record counts accurately. Additionally, the patch introduces error checking for
integer overflow during the summation of `local_records_read` to
`records_read`. Upon detecting an overflow, an appropriate error message is
displayed, and the program exits.

These improvements cater to scenarios where `ebizzy` is run with a large number
of threads over a substantial period. For example, when `ebizzy` is run for 60
seconds with 96 threads, and each thread processes between 5 to 6 million
records, the total record count can easily surpass `UINT_MAX`. This would lead
to an overflow. With these modifications, such situations are properly handled
and reported.

Vishal Chourasia (2):
  Enhanced thread safety in ebizzy benchmark tool
  Handle high record counts and prevent integer overflow

 utils/benchmark/ebizzy-0.3/ebizzy.c | 36 ++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
2.39.3


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

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

[Vishal Chourasia]

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


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

[LTP] [PATCH 2/2] Handle high record counts and prevent integer overflow

[Vishal Chourasia]

- The type of `records_read` is now `unsigned long long`, expanding the maximum
  record count.
- Implemented error check for integer overflow during the addition of
  `local_records_read` to `records_read`.
- Added error message and program exit upon detection of integer overflow.

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 | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/utils/benchmark/ebizzy-0.3/ebizzy.c b/utils/benchmark/ebizzy-0.3/ebizzy.c
index 1af004d9d..b89a0a97a 100644
--- a/utils/benchmark/ebizzy-0.3/ebizzy.c
+++ b/utils/benchmark/ebizzy-0.3/ebizzy.c
@@ -83,7 +83,7 @@ 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 unsigned long long records_read;
 
 pthread_mutex_t records_read_lock;
 
@@ -368,13 +368,13 @@ static inline unsigned int rand_num(unsigned int max, unsigned int *state)
  *
  */
 
-static unsigned int search_mem(void)
+static unsigned long long search_mem(void)
 {
 	record_t key, *found;
 	record_t *src, *copy;
 	unsigned int chunk;
 	size_t copy_size = chunk_size;
-	unsigned int i;
+	unsigned long long i;
 	unsigned int state = 0;
 
 	for (i = 0; threads_go == 1; i++) {
@@ -425,17 +425,29 @@ static unsigned int search_mem(void)
 
 static void *thread_run(void *arg __attribute__((unused)))
 {
+	unsigned long long old_records_read, local_records_read;
+
 	if (verbose > 1)
 		printf("Thread started\n");
 
 	/* Wait for the start signal */
-
 	while (threads_go == 0) ;
+	
+	local_records_read = search_mem();
 
 	pthread_mutex_lock(&records_read_lock);
-	records_read += search_mem();
+	old_records_read = records_read;
+	records_read += local_records_read;
 	pthread_mutex_unlock(&records_read_lock);
 
+	if (old_records_read > records_read || local_records_read > records_read) {
+		fprintf(stderr, "Error: Integer overflow occurred!\n");
+		fprintf(stderr, "Current records_read: %llu\n", records_read);
+		fprintf(stderr, "Local records_read: %llu\n", local_records_read);
+		fprintf(stderr, "Old records_read: %llu\n", old_records_read);
+		exit(1);
+	}
+
 	if (verbose > 1)
 		printf("Thread finished, %f seconds\n",
 		       difftime(time(NULL), start_time));
-- 
2.39.3


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

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

[Cyril Hrubis]

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

Re: [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
>>
> 


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