Commit cd2bcf10 authored by Mike Gerdts's avatar Mike Gerdts Committed by Jim Harris
Browse files

thread: SPDK spinlocks 



This introduces an enhanced spinlock that adds safeguards compared to
the default pthread_spinlock_t. In particular:

- A pthread_spinlock_t is still used, but additional error checking is
  performed to ensure there is no undefined behavior on relock,
  unlocking when not the owner, or destoying a locked lock.
- The SPDK concurrency model allows an SPDK thread to be migrated
  between pthreads. Releasing a pthread spinlock on a different thread
  from where it is taken is undefined behavior. If an SPDK spinlock is
  held at a time that a time when a poller or message returns control to
  thread_poll(), the program will abort.
- SPDK spinlocks can only be obtained from an SPDK thread.

Signed-off-by: default avatarMike Gerdts <mgerdts@nvidia.com>
Change-Id: I6dd6493ab5f5532ae69e20654546405a507eb594
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15277


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Community-CI: Mellanox Build Bot
parent 6733af8d

CHANGELOG.md

+3 −0
Original line number Diff line number Diff line
@@ -81,6 +81,9 @@ Added `spdk_thread_get_app_thread` which returns the first thread that was creat 
Added `spdk_thread_is_running`.  This returns `true` for a running thread, or `false` if

its exit process has started using `spdk_thread_exit`.



Added API `spdk_spin_init`, `spdk_spin_destroy`, `spdk_spin_lock`, `spdk_spin_unlock`, and

`spdk_spin_held` to support spinlocks that are aware of the SPDK concurrency model.



## v22.09



### accel

include/spdk/thread.h

+66 −0
Original line number Diff line number Diff line 
/*   SPDX-License-Identifier: BSD-3-Clause

 *   Copyright (C) 2016 Intel Corporation.

 *   All rights reserved.

 *   Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 */



/** \file
@@ -884,6 +885,71 @@ int spdk_interrupt_mode_enable(void); 
 */

bool spdk_interrupt_mode_is_enabled(void);



/**

 * A spinlock augmented with safety checks for use with SPDK.

 *

 * SPDK code that uses spdk_spinlock runs from an SPDK thread, which itself is associated with a

 * pthread. There are typically many SPDK threads associated with each pthread. The SPDK application

 * may migrate SPDK threads between pthreads from time to time to balance the load on those threads.

 * Migration of SPDK threads only happens when the thread is off CPU, and as such it is only safe to

 * hold a lock so long as an SPDK thread stays on CPU.

 *

 * It is not safe to lock a spinlock, return from the event or poller, then unlock it at some later

 * time because:

 *

 *   - Even though the SPDK thread may be the same, the SPDK thread may be running on different

 *     pthreads during lock and unlock. A pthread spinlock may consider this to be an unlock by a

 *     non-owner, which results in undefined behavior.

 *   - A lock that is acquired by a poller or event may be needed by another poller or event that

 *     runs on the same pthread. This can lead to deadlock or detection of deadlock.

 *   - A lock that is acquired by a poller or event that is needed by another poller or event that

 *     runs on a second pthread will block the second pthread from doing any useful work until the

 *     lock is released. Because the lock holder and the lock acquirer are on the same pthread, this

 *     would lead to deadlock.

 *

 * If an SPDK spinlock is used erroneously, the program will abort.

 */

struct spdk_spinlock {

	pthread_spinlock_t spinlock;

	struct spdk_thread *thread;

};



/**

 * Initialize an spdk_spinlock.

 *

 * \param sspin The SPDK spinlock to initialize.

 */

void spdk_spin_init(struct spdk_spinlock *sspin);



/**

 * Destroy an spdk_spinlock.

 *

 * \param sspin The SPDK spinlock to initialize.

 */

void spdk_spin_destroy(struct spdk_spinlock *sspin);



/**

 * Lock an SPDK spin lock.

 *

 * \param sspin An SPDK spinlock.

 */

void spdk_spin_lock(struct spdk_spinlock *sspin);



/**

 * Unlock an SPDK spinlock.

 *

 * \param sspin An SPDK spinlock.

 */

void spdk_spin_unlock(struct spdk_spinlock *sspin);



/**

 * Determine if the caller holds this SPDK spinlock.

 *

 * \param sspin An SPDK spinlock.

 * \return true if spinlock is held by this thread, else false

 */

bool spdk_spin_held(struct spdk_spinlock *sspin);



#ifdef __cplusplus

}

#endif

lib/thread/spdk_thread.map

+5 −0
Original line number Diff line number Diff line
@@ -57,6 +57,11 @@ 
	spdk_thread_get_interrupt_fd;

	spdk_interrupt_mode_enable;

	spdk_interrupt_mode_is_enabled;

	spdk_spin_init;

	spdk_spin_destroy;

	spdk_spin_lock;

	spdk_spin_unlock;

	spdk_spin_held;



	# internal functions in spdk_internal/thread.h

	spdk_poller_get_name;

lib/thread/thread.c

+132 −0
Original line number Diff line number Diff line 
/*   SPDX-License-Identifier: BSD-3-Clause

 *   Copyright (C) 2016 Intel Corporation.

 *   All rights reserved.

 *   Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 */



#include "spdk/stdinc.h"
@@ -129,6 +130,8 @@ struct spdk_thread { 
	struct spdk_cpuset		cpumask;

	uint64_t			exit_timeout_tsc;



	int32_t				lock_count;



	/* Indicates whether this spdk_thread currently runs in interrupt. */

	bool				in_interrupt;

	bool				poller_unregistered;
@@ -150,6 +153,64 @@ static size_t g_ctx_sz = 0; 
 */

static uint64_t g_thread_id = 1;



enum spin_error {

	SPIN_ERR_NONE,

	/* Trying to use an SPDK lock while not on an SPDK thread */

	SPIN_ERR_NOT_SPDK_THREAD,

	/* Trying to lock a lock already held by this SPDK thread */

	SPIN_ERR_DEADLOCK,

	/* Trying to unlock a lock not held by this SPDK thread */

	SPIN_ERR_WRONG_THREAD,

	/* pthread_spin_*() returned an error */

	SPIN_ERR_PTHREAD,

	/* Trying to destroy a lock that is held */

	SPIN_ERR_LOCK_HELD,

	/* lock_count is invalid */

	SPIN_ERR_LOCK_COUNT,

	/*

	 * An spdk_thread may migrate to another pthread. A spinlock held across migration leads to

	 * undefined behavior. A spinlock held when an SPDK thread goes off CPU would lead to

	 * deadlock when another SPDK thread on the same pthread tries to take that lock.

	 */

	SPIN_ERR_HOLD_DURING_SWITCH,

};



static const char *spin_error_strings[] = {

	[SPIN_ERR_NONE]			= "No error",

	[SPIN_ERR_NOT_SPDK_THREAD]	= "Not an SPDK thread",

	[SPIN_ERR_DEADLOCK]		= "Deadlock detected",

	[SPIN_ERR_WRONG_THREAD]		= "Unlock on wrong SPDK thread",

	[SPIN_ERR_PTHREAD]		= "Error from pthread_spinlock",

	[SPIN_ERR_LOCK_HELD]		= "Destroying a held spinlock",

	[SPIN_ERR_LOCK_COUNT]		= "Lock count is invalid",

	[SPIN_ERR_HOLD_DURING_SWITCH]	= "Lock(s) held while SPDK thread going off CPU",

};



#define SPIN_ERROR_STRING(err) (err < 0 || err >= SPDK_COUNTOF(spin_error_strings)) \

				? "Unknown error" : spin_error_strings[err]



static void

__posix_abort(enum spin_error err)

{

	abort();

}



typedef void (*spin_abort)(enum spin_error err);

spin_abort g_spin_abort_fn = __posix_abort;



#define SPIN_ASSERT_IMPL(cond, err, ret) \

	do { \

		if (spdk_unlikely(!(cond))) { \

			SPDK_ERRLOG("unrecoverable spinlock error %d: %s (%s)\n", err, \

				    SPIN_ERROR_STRING(err), #cond); \

			g_spin_abort_fn(err); \

			ret; \

		} \

	} while (0)

#define SPIN_ASSERT_RETURN_VOID(cond, err)	SPIN_ASSERT_IMPL(cond, err, return)

#define SPIN_ASSERT_RETURN(cond, err, ret)	SPIN_ASSERT_IMPL(cond, err, return ret)

#define SPIN_ASSERT(cond, err)			SPIN_ASSERT_IMPL(cond, err,)



struct io_device {

	void				*io_device;

	char				name[SPDK_MAX_DEVICE_NAME_LEN + 1];
@@ -726,6 +787,8 @@ msg_queue_run_batch(struct spdk_thread *thread, uint32_t max_msgs) 


		msg->fn(msg->arg);



		SPIN_ASSERT(thread->lock_count == 0, SPIN_ERR_HOLD_DURING_SWITCH);



		if (thread->msg_cache_count < SPDK_MSG_MEMPOOL_CACHE_SIZE) {

			/* Insert the messages at the head. We want to re-use the hot

			 * ones. */
@@ -829,6 +892,8 @@ thread_execute_poller(struct spdk_thread *thread, struct spdk_poller *poller) 
	poller->state = SPDK_POLLER_STATE_RUNNING;

	rc = poller->fn(poller->arg);



	SPIN_ASSERT(thread->lock_count == 0, SPIN_ERR_HOLD_DURING_SWITCH);



	poller->run_count++;

	if (rc > 0) {

		poller->busy_count++;
@@ -888,6 +953,8 @@ thread_execute_timed_poller(struct spdk_thread *thread, struct spdk_poller *poll 
	poller->state = SPDK_POLLER_STATE_RUNNING;

	rc = poller->fn(poller->arg);



	SPIN_ASSERT(thread->lock_count == 0, SPIN_ERR_HOLD_DURING_SWITCH);



	poller->run_count++;

	if (rc > 0) {

		poller->busy_count++;
@@ -1011,6 +1078,7 @@ spdk_thread_poll(struct spdk_thread *thread, uint32_t max_msgs, uint64_t now) 
	} else {

		/* Non-block wait on thread's fd_group */

		rc = spdk_fd_group_wait(thread->fgrp, 0);

		SPIN_ASSERT(thread->lock_count == 0, SPIN_ERR_HOLD_DURING_SWITCH);

		if (spdk_unlikely(!thread->in_interrupt)) {

			/* The thread transitioned to poll mode in a msg during the above processing.

			 * Clear msg_fd since thread messages will be polled directly in poll mode.
@@ -2745,4 +2813,68 @@ spdk_interrupt_mode_is_enabled(void) 
	return g_interrupt_mode;

}



void

spdk_spin_init(struct spdk_spinlock *sspin)

{

	int rc;



	memset(sspin, 0, sizeof(*sspin));

	rc = pthread_spin_init(&sspin->spinlock, PTHREAD_PROCESS_PRIVATE);

	SPIN_ASSERT_RETURN_VOID(rc == 0, SPIN_ERR_PTHREAD);

}



void

spdk_spin_destroy(struct spdk_spinlock *sspin)

{

	int rc;



	SPIN_ASSERT_RETURN_VOID(sspin->thread == NULL, SPIN_ERR_LOCK_HELD);



	rc = pthread_spin_destroy(&sspin->spinlock);

	SPIN_ASSERT_RETURN_VOID(rc == 0, SPIN_ERR_PTHREAD);

}



void

spdk_spin_lock(struct spdk_spinlock *sspin)

{

	struct spdk_thread *thread = spdk_get_thread();

	int rc;



	SPIN_ASSERT_RETURN_VOID(thread != NULL, SPIN_ERR_NOT_SPDK_THREAD);

	SPIN_ASSERT_RETURN_VOID(thread != sspin->thread, SPIN_ERR_DEADLOCK);



	rc = pthread_spin_lock(&sspin->spinlock);

	SPIN_ASSERT_RETURN_VOID(rc == 0, SPIN_ERR_PTHREAD);



	sspin->thread = thread;

	sspin->thread->lock_count++;

}



void

spdk_spin_unlock(struct spdk_spinlock *sspin)

{

	struct spdk_thread *thread = spdk_get_thread();

	int rc;



	SPIN_ASSERT_RETURN_VOID(thread != NULL, SPIN_ERR_NOT_SPDK_THREAD);

	SPIN_ASSERT_RETURN_VOID(thread == sspin->thread, SPIN_ERR_WRONG_THREAD);



	SPIN_ASSERT_RETURN_VOID(thread->lock_count > 0, SPIN_ERR_LOCK_COUNT);

	thread->lock_count--;

	sspin->thread = NULL;



	rc = pthread_spin_unlock(&sspin->spinlock);

	SPIN_ASSERT_RETURN_VOID(rc == 0, SPIN_ERR_PTHREAD);

}



bool

spdk_spin_held(struct spdk_spinlock *sspin)

{

	struct spdk_thread *thread = spdk_get_thread();



	SPIN_ASSERT_RETURN(thread != NULL, SPIN_ERR_NOT_SPDK_THREAD, false);



	return sspin->thread == thread;

}



SPDK_LOG_REGISTER_COMPONENT(thread)

test/unit/lib/thread/thread.c/thread_ut.c

+137 −0
Original line number Diff line number Diff line 
/*   SPDX-License-Identifier: BSD-3-Clause

 *   Copyright (C) 2016 Intel Corporation.

 *   All rights reserved.

 *   Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 */



#include "spdk/stdinc.h"
@@ -1777,6 +1778,141 @@ io_device_lookup(void) 
	free_threads();

}



static enum spin_error g_spin_err;

static uint32_t g_spin_err_count = 0;



static void

ut_track_abort(enum spin_error err)

{

	g_spin_err = err;

	g_spin_err_count++;

}



static void

spdk_spin(void)

{

	struct spdk_spinlock lock;



	g_spin_abort_fn = ut_track_abort;



	/* Do not need to be on an SPDK thread to initialize an spdk_spinlock */

	g_spin_err_count = 0;

	spdk_spin_init(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Trying to take a lock while not on an SPDK thread is an error */

	g_spin_err_count = 0;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_NOT_SPDK_THREAD);



	/* Trying to check if a lock is held while not on an SPDK thread is an error */

	g_spin_err_count = 0;

	spdk_spin_held(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_NOT_SPDK_THREAD);



	/* Do not need to be on an SPDK thread to destroy an spdk_spinlock */

	g_spin_err_count = 0;

	spdk_spin_destroy(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	allocate_threads(2);

	set_thread(0);



	/* Can initialize an spdk_spinlock on an SPDK thread */

	g_spin_err_count = 0;

	spdk_spin_init(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Can take spinlock */

	g_spin_err_count = 0;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Can release spinlock */

	g_spin_err_count = 0;

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Deadlock detected */

	g_spin_err_count = 0;

	g_spin_err = SPIN_ERR_NONE;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_DEADLOCK);



	/* Cannot unlock from wrong thread */

	set_thread(1);

	g_spin_err_count = 0;

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_WRONG_THREAD);



	/* Get back to a known good state */

	set_thread(0);

	g_spin_err_count = 0;

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Cannot release the same lock twice */

	g_spin_err_count = 0;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_WRONG_THREAD);



	/* A lock that is not held is properly recognized */

	g_spin_err_count = 0;

	CU_ASSERT(!spdk_spin_held(&lock));

	CU_ASSERT(g_spin_err_count == 0);



	/* A lock that is held is recognized as held by only the thread that holds it. */

	set_thread(1);

	g_spin_err_count = 0;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	CU_ASSERT(spdk_spin_held(&lock));

	CU_ASSERT(g_spin_err_count == 0);

	set_thread(0);

	CU_ASSERT(!spdk_spin_held(&lock));

	CU_ASSERT(g_spin_err_count == 0);



	/* After releasing, no one thinks it is held */

	set_thread(1);

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	CU_ASSERT(!spdk_spin_held(&lock));

	CU_ASSERT(g_spin_err_count == 0);

	set_thread(0);

	CU_ASSERT(!spdk_spin_held(&lock));

	CU_ASSERT(g_spin_err_count == 0);



	/* Destroying a lock that is held is an error. */

	set_thread(0);

	g_spin_err_count = 0;

	spdk_spin_lock(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	spdk_spin_destroy(&lock);

	CU_ASSERT(g_spin_err_count == 1);

	CU_ASSERT(g_spin_err == SPIN_ERR_LOCK_HELD);

	g_spin_err_count = 0;

	spdk_spin_unlock(&lock);

	CU_ASSERT(g_spin_err_count == 0);



	/* Clean up */

	g_spin_err_count = 0;

	spdk_spin_destroy(&lock);

	CU_ASSERT(g_spin_err_count == 0);

	free_threads();

	g_spin_abort_fn = __posix_abort;

}



int

main(int argc, char **argv)

{
@@ -1805,6 +1941,7 @@ main(int argc, char **argv) 
	CU_ADD_TEST(suite, cache_closest_timed_poller);

	CU_ADD_TEST(suite, multi_timed_pollers_have_same_expiration);

	CU_ADD_TEST(suite, io_device_lookup);

	CU_ADD_TEST(suite, spdk_spin);



	CU_basic_set_mode(CU_BRM_VERBOSE);

	CU_basic_run_tests();