From bc9a36bad14b014340244bfc35a20df6809a5568 Mon Sep 17 00:00:00 2001

From: Miroslav Lichvar <mlichvar@redhat.com>

Date: Thu, 27 Feb 2020 15:35:31 +0100

Subject: [PATCH] Fix rwlock to be thread-safe

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

This is a backport of the following commits

commit 4cf275ba8aa1caf47ed763b51c37fa561005cb8d

Author: Ondřej Surý <ondrej@isc.org>

Date:   Wed Feb 12 09:17:55 2020 +0100

    Replace non-loop usage of atomic_compare_exchange_weak with strong variant

commit b43f5e023885dac9f1ffdace54720150768a333b

Author: Ondřej Surý <ondrej@isc.org>

Date:   Sat Feb 1 10:48:20 2020 +0100

    Convert all atomic operations in isc_rwlock to release-acquire memory ordering

commit 49462cf9747261cbc39d5fa4c691b64ac5472af4

Author: Ondřej Surý <ondrej@sury.org>

Date:   Tue May 14 00:19:11 2019 +0700

    Make isc_rwlock.c thread-safe

commit 9d5df99a9d9d13c9487969b6fa3818a8b83b4ee2

Author: Ondřej Surý <ondrej@sury.org>

Date:   Thu Aug 23 15:30:06 2018 +0200

    Directly use return value of atomic_compare_exchange_strong_explicit insteaf of comparing expected value

commit b5709e5531d9d45f9fc3db129c11ad474477d7b6

Author: Ondřej Surý <ondrej@sury.org>

Date:   Fri Aug 17 19:21:12 2018 +0200

    Explicitly load atomic values in lib/isc/rwlock.c

---

 lib/isc/rwlock.c | 275 ++++++++++++++++++-----------------------------

 1 file changed, 107 insertions(+), 168 deletions(-)

diff --git a/lib/isc/rwlock.c b/lib/isc/rwlock.c

index 9533c0f828..5591eff719 100644

--- a/lib/isc/rwlock.c

+++ b/lib/isc/rwlock.c

@@ -46,6 +46,26 @@

 #if defined(ISC_RWLOCK_USEATOMIC)

 static isc_result_t

 isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type);

+

+#ifndef ISC_RWLOCK_USESTDATOMIC

+#error non-stdatomic support removed

+#endif

+

+#define atomic_load_acquire(o)						\

+	atomic_load_explicit((o), memory_order_acquire)

+#define atomic_store_release(o, v)					\

+	atomic_store_explicit((o), (v), memory_order_release)

+#define atomic_fetch_add_release(o, v)					\

+	atomic_fetch_add_explicit((o), (v), memory_order_release)

+#define atomic_fetch_sub_release(o, v)					\

+	atomic_fetch_sub_explicit((o), (v), memory_order_release)

+#define atomic_compare_exchange_weak_acq_rel(o, e, d)			\

+	atomic_compare_exchange_weak_explicit((o), (e), (d),		\

+					      memory_order_acq_rel,	\

+					      memory_order_acquire)

+#define atomic_compare_exchange_strong_acq_rel(o, e, d)			\

+	atomic_compare_exchange_strong_explicit(			\

+		(o), (e), (d), memory_order_acq_rel, memory_order_acquire)

 #endif

 #ifdef ISC_RWLOCK_TRACE

@@ -108,13 +128,13 @@ isc_rwlock_init(isc_rwlock_t *rwl, unsigned int read_quota,

 	 */

 	rwl->magic = 0;

-	rwl->spins = 0;

 #if defined(ISC_RWLOCK_USEATOMIC)

-	rwl->write_requests = 0;

-	rwl->write_completions = 0;

-	rwl->cnt_and_flag = 0;

+	atomic_init(&rwl->spins, 0);

+	atomic_init(&rwl->write_requests, 0);

+	atomic_init(&rwl->write_completions, 0);

+	atomic_init(&rwl->cnt_and_flag, 0);

 	rwl->readers_waiting = 0;

-	rwl->write_granted = 0;

+	atomic_init(&rwl->write_granted, 0);

 	if (read_quota != 0) {

 		UNEXPECTED_ERROR(__FILE__, __LINE__,

 				 "read quota is not supported");

@@ -123,6 +143,7 @@ isc_rwlock_init(isc_rwlock_t *rwl, unsigned int read_quota,

 		write_quota = RWLOCK_DEFAULT_WRITE_QUOTA;

 	rwl->write_quota = write_quota;

 #else

+	rwl->spins = 0;

 	rwl->type = isc_rwlocktype_read;

 	rwl->original = isc_rwlocktype_none;

 	rwl->active = 0;

@@ -178,16 +199,9 @@ void

 isc_rwlock_destroy(isc_rwlock_t *rwl) {

 	REQUIRE(VALID_RWLOCK(rwl));

-#if defined(ISC_RWLOCK_USEATOMIC)

-	REQUIRE(rwl->write_requests == rwl->write_completions &&

-		rwl->cnt_and_flag == 0 && rwl->readers_waiting == 0);

-#else

-	LOCK(&rwl->lock);

-	REQUIRE(rwl->active == 0 &&

-		rwl->readers_waiting == 0 &&

-		rwl->writers_waiting == 0);

-	UNLOCK(&rwl->lock);

-#endif

+	REQUIRE(atomic_load_acquire(&rwl->write_requests) ==

+		atomic_load_acquire(&rwl->write_completions) &&

+		atomic_load_acquire(&rwl->cnt_and_flag) == 0 && rwl->readers_waiting == 0);

 	rwl->magic = 0;

 	(void)isc_condition_destroy(&rwl->readable);

@@ -274,10 +288,13 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 #endif

 	if (type == isc_rwlocktype_read) {

-		if (rwl->write_requests != rwl->write_completions) {

+		if (atomic_load_acquire(&rwl->write_requests) !=

+		    atomic_load_acquire(&rwl->write_completions))

+		{

 			/* there is a waiting or active writer */

 			LOCK(&rwl->lock);

-			if (rwl->write_requests != rwl->write_completions) {

+			if (atomic_load_acquire(&rwl->write_requests) !=

+			    atomic_load_acquire(&rwl->write_completions)) {

 				rwl->readers_waiting++;

 				WAIT(&rwl->readable, &rwl->lock);

 				rwl->readers_waiting--;

@@ -285,23 +302,24 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 			UNLOCK(&rwl->lock);

 		}

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		cntflag = atomic_fetch_add_explicit(&rwl->cnt_and_flag,

-						    READER_INCR,

-						    memory_order_relaxed);

-#else

-		cntflag = isc_atomic_xadd(&rwl->cnt_and_flag, READER_INCR);

-#endif

+		cntflag = atomic_fetch_add_release(&rwl->cnt_and_flag,

+						   READER_INCR);

 		POST(cntflag);

 		while (1) {

-			if ((rwl->cnt_and_flag & WRITER_ACTIVE) == 0)

+			if ((atomic_load_acquire(&rwl->cnt_and_flag)

+			     & WRITER_ACTIVE) == 0)

+			{

 				break;

+			}

 			/* A writer is still working */

 			LOCK(&rwl->lock);

 			rwl->readers_waiting++;

-			if ((rwl->cnt_and_flag & WRITER_ACTIVE) != 0)

+			if ((atomic_load_acquire(&rwl->cnt_and_flag)

+			     & WRITER_ACTIVE) != 0)

+			{

 				WAIT(&rwl->readable, &rwl->lock);

+			}

 			rwl->readers_waiting--;

 			UNLOCK(&rwl->lock);

@@ -336,20 +354,19 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 		 * quota, reset the condition (race among readers doesn't

 		 * matter).

 		 */

-		rwl->write_granted = 0;

+		atomic_store_release(&rwl->write_granted, 0);

 	} else {

 		int32_t prev_writer;

 		/* enter the waiting queue, and wait for our turn */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		prev_writer = atomic_fetch_add_explicit(&rwl->write_requests, 1,

-							memory_order_relaxed);

-#else

-		prev_writer = isc_atomic_xadd(&rwl->write_requests, 1);

-#endif

-		while (rwl->write_completions != prev_writer) {

+		prev_writer = atomic_fetch_add_release(&rwl->write_requests, 1);

+		while (atomic_load_acquire(&rwl->write_completions)

+		       != prev_writer)

+		{

 			LOCK(&rwl->lock);

-			if (rwl->write_completions != prev_writer) {

+			if (atomic_load_acquire(&rwl->write_completions)

+			    != prev_writer)

+			{

 				WAIT(&rwl->writeable, &rwl->lock);

 				UNLOCK(&rwl->lock);

 				continue;

@@ -359,29 +376,24 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 		}

 		while (1) {

-#if defined(ISC_RWLOCK_USESTDATOMIC)

 			int_fast32_t cntflag2 = 0;

-			atomic_compare_exchange_strong_explicit

-				(&rwl->cnt_and_flag, &cntflag2, WRITER_ACTIVE,

-				 memory_order_relaxed, memory_order_relaxed);

-#else

-			int32_t cntflag2;

-			cntflag2 = isc_atomic_cmpxchg(&rwl->cnt_and_flag, 0,

-						      WRITER_ACTIVE);

-#endif

-

-			if (cntflag2 == 0)

+			if (atomic_compare_exchange_weak_acq_rel(

+				    &rwl->cnt_and_flag, &cntflag2, WRITER_ACTIVE))

+			{

 				break;

+			}

 			/* Another active reader or writer is working. */

 			LOCK(&rwl->lock);

-			if (rwl->cnt_and_flag != 0)

+			if (atomic_load_acquire(&rwl->cnt_and_flag) != 0) {

 				WAIT(&rwl->writeable, &rwl->lock);

+			}

 			UNLOCK(&rwl->lock);

 		}

-		INSIST((rwl->cnt_and_flag & WRITER_ACTIVE) != 0);

-		rwl->write_granted++;

+		INSIST((atomic_load_acquire(&rwl->cnt_and_flag)

+			& WRITER_ACTIVE));

+		atomic_fetch_add_release(&rwl->write_granted, 1);

 	}

 #ifdef ISC_RWLOCK_TRACE

@@ -395,12 +407,10 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 isc_result_t

 isc_rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 	int32_t cnt = 0;

-	int32_t max_cnt = rwl->spins * 2 + 10;

+	int32_t spins = atomic_load_acquire(&rwl->spins) * 2 + 10;

+	int32_t max_cnt = ISC_MAX(spins, RWLOCK_MAX_ADAPTIVE_COUNT);

 	isc_result_t result = ISC_R_SUCCESS;

-	if (max_cnt > RWLOCK_MAX_ADAPTIVE_COUNT)

-		max_cnt = RWLOCK_MAX_ADAPTIVE_COUNT;

-

 	do {

 		if (cnt++ >= max_cnt) {

 			result = isc__rwlock_lock(rwl, type);

@@ -411,7 +421,7 @@ isc_rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 #endif

 	} while (isc_rwlock_trylock(rwl, type) != ISC_R_SUCCESS);

-	rwl->spins += (cnt - rwl->spins) / 8;

+	atomic_fetch_add_release(&rwl->spins, (cnt - spins) / 8);

 	return (result);

 }

@@ -429,36 +439,28 @@ isc_rwlock_trylock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 	if (type == isc_rwlocktype_read) {

 		/* If a writer is waiting or working, we fail. */

-		if (rwl->write_requests != rwl->write_completions)

+		if (atomic_load_acquire(&rwl->write_requests) !=

+		    atomic_load_acquire(&rwl->write_completions))

 			return (ISC_R_LOCKBUSY);

 		/* Otherwise, be ready for reading. */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		cntflag = atomic_fetch_add_explicit(&rwl->cnt_and_flag,

-						    READER_INCR,

-						    memory_order_relaxed);

-#else

-		cntflag = isc_atomic_xadd(&rwl->cnt_and_flag, READER_INCR);

-#endif

+		cntflag = atomic_fetch_add_release(&rwl->cnt_and_flag,

+						   READER_INCR);

 		if ((cntflag & WRITER_ACTIVE) != 0) {

 			/*

 			 * A writer is working.  We lose, and cancel the read

 			 * request.

 			 */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-			cntflag = atomic_fetch_sub_explicit

-				(&rwl->cnt_and_flag, READER_INCR,

-				 memory_order_relaxed);

-#else

-			cntflag = isc_atomic_xadd(&rwl->cnt_and_flag,

-						  -READER_INCR);

-#endif

+			cntflag = atomic_fetch_sub_release(

+				&rwl->cnt_and_flag, READER_INCR);

 			/*

 			 * If no other readers are waiting and we've suspended

 			 * new writers in this short period, wake them up.

 			 */

 			if (cntflag == READER_INCR &&

-			    rwl->write_completions != rwl->write_requests) {

+			    atomic_load_acquire(&rwl->write_completions) !=

+			    atomic_load_acquire(&rwl->write_requests))

+			{

 				LOCK(&rwl->lock);

 				BROADCAST(&rwl->writeable);

 				UNLOCK(&rwl->lock);

@@ -468,31 +470,19 @@ isc_rwlock_trylock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 		}

 	} else {

 		/* Try locking without entering the waiting queue. */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

 		int_fast32_t zero = 0;

-		if (!atomic_compare_exchange_strong_explicit

-		    (&rwl->cnt_and_flag, &zero, WRITER_ACTIVE,

-		     memory_order_relaxed, memory_order_relaxed))

+		if (!atomic_compare_exchange_strong_acq_rel(

+		    &rwl->cnt_and_flag, &zero, WRITER_ACTIVE))

+		{

 			return (ISC_R_LOCKBUSY);

-#else

-		cntflag = isc_atomic_cmpxchg(&rwl->cnt_and_flag, 0,

-					     WRITER_ACTIVE);

-		if (cntflag != 0)

-			return (ISC_R_LOCKBUSY);

-#endif

+		}

 		/*

 		 * XXXJT: jump into the queue, possibly breaking the writer

 		 * order.

 		 */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		atomic_fetch_sub_explicit(&rwl->write_completions, 1,

-					  memory_order_relaxed);

-#else

-		(void)isc_atomic_xadd(&rwl->write_completions, -1);

-#endif

-

-		rwl->write_granted++;

+		atomic_fetch_sub_release(&rwl->write_completions, 1);

+		atomic_fetch_add_release(&rwl->write_granted, 1);

 	}

 #ifdef ISC_RWLOCK_TRACE

@@ -507,14 +497,12 @@ isc_result_t

 isc_rwlock_tryupgrade(isc_rwlock_t *rwl) {

 	REQUIRE(VALID_RWLOCK(rwl));

-#if defined(ISC_RWLOCK_USESTDATOMIC)

 	{

 		int_fast32_t reader_incr = READER_INCR;

 		/* Try to acquire write access. */

-		atomic_compare_exchange_strong_explicit

-			(&rwl->cnt_and_flag, &reader_incr, WRITER_ACTIVE,

-			 memory_order_relaxed, memory_order_relaxed);

+		atomic_compare_exchange_strong_acq_rel(

+			&rwl->cnt_and_flag, &reader_incr, WRITER_ACTIVE);

 		/*

 		 * There must have been no writer, and there must have

 		 * been at least one reader.

@@ -527,36 +515,11 @@ isc_rwlock_tryupgrade(isc_rwlock_t *rwl) {

 			 * We are the only reader and have been upgraded.

 			 * Now jump into the head of the writer waiting queue.

 			 */

-			atomic_fetch_sub_explicit(&rwl->write_completions, 1,

-						  memory_order_relaxed);

+			atomic_fetch_sub_release(&rwl->write_completions, 1);

 		} else

 			return (ISC_R_LOCKBUSY);

 	}

-#else

-	{

-		int32_t prevcnt;

-

-		/* Try to acquire write access. */

-		prevcnt = isc_atomic_cmpxchg(&rwl->cnt_and_flag,

-					     READER_INCR, WRITER_ACTIVE);

-		/*

-		 * There must have been no writer, and there must have

-		 * been at least one reader.

-		 */

-		INSIST((prevcnt & WRITER_ACTIVE) == 0 &&

-		       (prevcnt & ~WRITER_ACTIVE) != 0);

-

-		if (prevcnt == READER_INCR) {

-			/*

-			 * We are the only reader and have been upgraded.

-			 * Now jump into the head of the writer waiting queue.

-			 */

-			(void)isc_atomic_xadd(&rwl->write_completions, -1);

-		} else

-			return (ISC_R_LOCKBUSY);

-	}

-#endif

 	return (ISC_R_SUCCESS);

 }

@@ -567,33 +530,15 @@ isc_rwlock_downgrade(isc_rwlock_t *rwl) {

 	REQUIRE(VALID_RWLOCK(rwl));

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-	{

-		/* Become an active reader. */

-		prev_readers = atomic_fetch_add_explicit(&rwl->cnt_and_flag,

-							 READER_INCR,

-							 memory_order_relaxed);

-		/* We must have been a writer. */

-		INSIST((prev_readers & WRITER_ACTIVE) != 0);

-

-		/* Complete write */

-		atomic_fetch_sub_explicit(&rwl->cnt_and_flag, WRITER_ACTIVE,

-					  memory_order_relaxed);

-		atomic_fetch_add_explicit(&rwl->write_completions, 1,

-					  memory_order_relaxed);

-	}

-#else

-	{

-		/* Become an active reader. */

-		prev_readers = isc_atomic_xadd(&rwl->cnt_and_flag, READER_INCR);

-		/* We must have been a writer. */

-		INSIST((prev_readers & WRITER_ACTIVE) != 0);

-

-		/* Complete write */

-		(void)isc_atomic_xadd(&rwl->cnt_and_flag, -WRITER_ACTIVE);

-		(void)isc_atomic_xadd(&rwl->write_completions, 1);

-	}

-#endif

+	/* Become an active reader. */

+	prev_readers = atomic_fetch_add_release(&rwl->cnt_and_flag,

+						READER_INCR);

+	/* We must have been a writer. */

+	INSIST((prev_readers & WRITER_ACTIVE) != 0);

+

+	/* Complete write */

+	atomic_fetch_sub_release(&rwl->cnt_and_flag, WRITER_ACTIVE);

+	atomic_fetch_add_release(&rwl->write_completions, 1);

 	/* Resume other readers */

 	LOCK(&rwl->lock);

@@ -614,20 +559,16 @@ isc_rwlock_unlock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 #endif

 	if (type == isc_rwlocktype_read) {

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		prev_cnt = atomic_fetch_sub_explicit(&rwl->cnt_and_flag,

-						     READER_INCR,

-						     memory_order_relaxed);

-#else

-		prev_cnt = isc_atomic_xadd(&rwl->cnt_and_flag, -READER_INCR);

-#endif

+		prev_cnt = atomic_fetch_sub_release(&rwl->cnt_and_flag,

+						    READER_INCR);

 		/*

 		 * If we're the last reader and any writers are waiting, wake

 		 * them up.  We need to wake up all of them to ensure the

 		 * FIFO order.

 		 */

 		if (prev_cnt == READER_INCR &&

-		    rwl->write_completions != rwl->write_requests) {

+		    atomic_load_acquire(&rwl->write_completions) !=

+		    atomic_load_acquire(&rwl->write_requests)) {

 			LOCK(&rwl->lock);

 			BROADCAST(&rwl->writeable);

 			UNLOCK(&rwl->lock);

@@ -639,19 +580,16 @@ isc_rwlock_unlock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 		 * Reset the flag, and (implicitly) tell other writers

 		 * we are done.

 		 */

-#if defined(ISC_RWLOCK_USESTDATOMIC)

-		atomic_fetch_sub_explicit(&rwl->cnt_and_flag, WRITER_ACTIVE,

-					  memory_order_relaxed);

-		atomic_fetch_add_explicit(&rwl->write_completions, 1,

-					  memory_order_relaxed);

-#else

-		(void)isc_atomic_xadd(&rwl->cnt_and_flag, -WRITER_ACTIVE);

-		(void)isc_atomic_xadd(&rwl->write_completions, 1);

-#endif

-

-		if (rwl->write_granted >= rwl->write_quota ||

-		    rwl->write_requests == rwl->write_completions ||

-		    (rwl->cnt_and_flag & ~WRITER_ACTIVE) != 0) {

+		atomic_fetch_sub_release(&rwl->cnt_and_flag, WRITER_ACTIVE);

+		atomic_fetch_add_release(&rwl->write_completions, 1);

+

+		if ((atomic_load_acquire(&rwl->write_granted) >=

+		     rwl->write_quota) ||

+		    (atomic_load_acquire(&rwl->write_requests) ==

+		    atomic_load_acquire(&rwl->write_completions)) ||

+		    (atomic_load_acquire(&rwl->cnt_and_flag)

+		     & ~WRITER_ACTIVE))

+		{

 			/*

 			 * We have passed the write quota, no writer is

 			 * waiting, or some readers are almost ready, pending

@@ -668,7 +606,8 @@ isc_rwlock_unlock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {

 			UNLOCK(&rwl->lock);

 		}

-		if (rwl->write_requests != rwl->write_completions &&

+		if ((atomic_load_acquire(&rwl->write_requests) !=

+		     atomic_load_acquire(&rwl->write_completions)) &&

 		    wakeup_writers) {

 			LOCK(&rwl->lock);

 			BROADCAST(&rwl->writeable);

-- 

2.21.0