NetBSD Problem Report #55677

From www@netbsd.org  Mon Sep 21 17:24:47 2020
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 704BF1A9217
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 21 Sep 2020 17:24:47 +0000 (UTC)
Message-Id: <20200921172446.463F51A9239@mollari.NetBSD.org>
Date: Mon, 21 Sep 2020 17:24:46 +0000 (UTC)
From: arichardson@freebsd.org
Reply-To: arichardson@freebsd.org
To: gnats-bugs@NetBSD.org
Subject: Significantly speed up libthr/mutex_test and make more reliable
X-Send-Pr-Version: www-1.0

>Number:         55677
>Category:       bin
>Synopsis:       Significantly speed up libthr/mutex_test and make more reliable
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Mon Sep 21 17:25:00 +0000 2020
>Originator:     Alex Richardson
>Release:        
>Organization:
FreeBSD
>Environment:
>Description:
Instead of using a simple global++ as the data race, with this change we
perform the increment by loading the global, delaying for a bit and then
storing back the incremented value. If I move the increment outside of the
mutex protected range, I can now see the data race with only 100 iterations
on amd64 in almost all cases. Before this change this almost always passed
with < 100,000 iterations and only reliably failed with the current
limit of 10 million.

I noticed this issue because the mutex:mutex{2,3} and timedmutex:mutex{2,3}
tests were always timing out on RISC-V QEMU in our CheriBSD Jenkins.
This change should make the test more likely to fail if pthread_mutex_lock
is not implemented correctly while also significantly reducing the time it
takes to run these four tests.
>How-To-Repeat:
Run t_mutex.c on fully-emulated QEMU (e.g. RISC-V).
>Fix:
diff --git a/tests/lib/libpthread/t_mutex.c b/tests/lib/libpthread/t_mutex.c
index ef277fa7d..ba8a7de4b 100644
--- a/tests/lib/libpthread/t_mutex.c
+++ b/tests/lib/libpthread/t_mutex.c
@@ -128,16 +128,33 @@ ATF_TC_BODY(mutex1, tc)
 	PTHREAD_REQUIRE(pthread_mutex_unlock(&mutex));
 }

+/*
+ * Increment the value using a noinline function that includes a small delay
+ * to increase the window for the RMW data race.
+ */
+__noinline static int
+increment(int value)
+{
+	for (volatile int i = 0; i < 100; i++) {
+		/* Small delay between read+write to increase chance of race */
+		__asm__ volatile("");
+	}
+	return value + 1;
+}
+
+static volatile bool thread2_started = false;
+
 static void *
 mutex2_threadfunc(void *arg)
 {
 	long count = *(int *)arg;

+	thread2_started = true;
 	printf("2: Second thread (%p). Count is %ld\n", pthread_self(), count);

 	while (count--) {
 		PTHREAD_REQUIRE(mutex_lock(&mutex, &ts_lengthy));
-		global_x++;
+		global_x = increment(global_x);
 		PTHREAD_REQUIRE(pthread_mutex_unlock(&mutex));
 	}

@@ -151,7 +168,7 @@ ATF_TC_HEAD(mutex2, tc)
 }
 ATF_TC_BODY(mutex2, tc)
 {
-	int count, count2;
+	int count, count2, num_increments;
 	pthread_t new;
 	void *joinval;

@@ -160,18 +177,27 @@ ATF_TC_BODY(mutex2, tc)
 	PTHREAD_REQUIRE(pthread_mutex_init(&mutex, NULL));

 	global_x = 0;
-	count = count2 = 10000000;
+	num_increments = count = count2 = 1000;
+	if (getenv("NUM_ITERATIONS") != NULL) {
+		num_increments = count = count2 = atoi(getenv("NUM_ITERATIONS"));
+	}
+	printf("Will use %d iterations\n", num_increments);
+

 	PTHREAD_REQUIRE(mutex_lock(&mutex, &ts_lengthy));
+	thread2_started = false;
 	PTHREAD_REQUIRE(pthread_create(&new, NULL, mutex2_threadfunc, &count2));

 	printf("1: Thread %p\n", pthread_self());
-
+	while (!thread2_started) {
+		/* Wait for thread 2 to start to increase chance of race */
+	}
+	printf("1: Unlocking to start increment loop %p\n", pthread_self());
 	PTHREAD_REQUIRE(pthread_mutex_unlock(&mutex));

 	while (count--) {
 		PTHREAD_REQUIRE(mutex_lock(&mutex, &ts_lengthy));
-		global_x++;
+		global_x = increment(global_x);
 		PTHREAD_REQUIRE(pthread_mutex_unlock(&mutex));
 	}

@@ -180,19 +206,25 @@ ATF_TC_BODY(mutex2, tc)
 	PTHREAD_REQUIRE(mutex_lock(&mutex, &ts_lengthy));
 	printf("1: Thread joined. X was %d. Return value (long) was %ld\n",
 		global_x, (long)joinval);
-	ATF_REQUIRE_EQ(global_x, 20000000);
+	ATF_REQUIRE_EQ_MSG(count, -1, "%d", count);
+	ATF_REQUIRE_EQ_MSG((long)joinval, -1, "%ld", (long)joinval);
+	ATF_REQUIRE_EQ_MSG(global_x, num_increments * 2, "%d vs %d", global_x,
+	    num_increments * 2);
 }

+static volatile bool thread3_started = false;
+
 static void *
 mutex3_threadfunc(void *arg)
 {
 	long count = *(int *)arg;

+	thread3_started = true;
 	printf("2: Second thread (%p). Count is %ld\n", pthread_self(), count);

 	while (count--) {
 		PTHREAD_REQUIRE(mutex_lock(&static_mutex, &ts_lengthy));
-		global_x++;
+		global_x = increment(global_x);
 		PTHREAD_REQUIRE(pthread_mutex_unlock(&static_mutex));
 	}

@@ -207,25 +239,32 @@ ATF_TC_HEAD(mutex3, tc)
 }
 ATF_TC_BODY(mutex3, tc)
 {
-	int count, count2;
+	int count, count2, num_increments;
 	pthread_t new;
 	void *joinval;

 	printf("1: Mutex-test 3\n");

 	global_x = 0;
-	count = count2 = 10000000;
+	num_increments = count = count2 = 1000;
+	if (getenv("NUM_ITERATIONS") != NULL) {
+		num_increments = count = count2 = atoi(getenv("NUM_ITERATIONS"));
+	}
+	printf("Will use %d iterations\n", num_increments);

 	PTHREAD_REQUIRE(mutex_lock(&static_mutex, &ts_lengthy));
 	PTHREAD_REQUIRE(pthread_create(&new, NULL, mutex3_threadfunc, &count2));

 	printf("1: Thread %p\n", pthread_self());
-
+	while (!thread3_started) {
+		/* Wait for thread 3 to start to increase chance of race */
+	}
+	printf("1: Unlocking to start increment loop %p\n", pthread_self());
 	PTHREAD_REQUIRE(pthread_mutex_unlock(&static_mutex));

 	while (count--) {
 		PTHREAD_REQUIRE(mutex_lock(&static_mutex, &ts_lengthy));
-		global_x++;
+		global_x = increment(global_x);
 		PTHREAD_REQUIRE(pthread_mutex_unlock(&static_mutex));
 	}

@@ -234,7 +273,10 @@ ATF_TC_BODY(mutex3, tc)
 	PTHREAD_REQUIRE(mutex_lock(&static_mutex, &ts_lengthy));
 	printf("1: Thread joined. X was %d. Return value (long) was %ld\n",
 		global_x, (long)joinval);
-	ATF_REQUIRE_EQ(global_x, 20000000);
+	ATF_REQUIRE_EQ_MSG(count, -1, "%d", count);
+	ATF_REQUIRE_EQ_MSG((long)joinval, -1, "%ld", (long)joinval);
+	ATF_REQUIRE_EQ_MSG(global_x, num_increments * 2, "%d vs %d", global_x,
+	    num_increments * 2);
 }

 static void *

Home	PR Database Search