diff --git a/TODO b/TODO index ecce47baf5..1f2820f073 100644 --- a/TODO +++ b/TODO @@ -14,7 +14,7 @@ nuttx/: (1) Memory Management (mm/) (0) Power Management (drivers/pm) (5) Signals (sched/signal, arch/) - (3) pthreads (sched/pthread, libs/libc/pthread) + (2) pthreads (sched/pthread, libs/libc/pthread) (0) Message Queues (sched/mqueue) (10) Kernel/Protected Build (3) C++ Support @@ -896,42 +896,6 @@ o pthreads (sched/pthreads libs/libc/pthread) Priority: Low. This change would improve real-time performance of the OS but is not otherwise required. - Title: PTHREAD SPINLOCK ISSUES - Description: Basic support for pthread spinlocks was added on 2019-02-28, - However, there are some issues related to the implementation - as it stands. - - 1. The Use is not fully verified. How are these interfaces - used? How should they be tested? - - 2. These interfaces depend on architecture specific support - support fraom each architecture that support is not be - fully available to the pthread library in all cases. - - There should a setting, say CONFIG_ARCH_HAVE_TESTSET=y - that indicates if the architect provides up_testset(). - - 3. It is also restricted to CONFIG_BUILD_FLAT because the - critical test and set function (up_testset()) as - prototyped in include/nuttx/spinlock() does not permit an - inline function or macro to be used in user-mode - application space. - - Update: A better alternative would be to move the CPU- - depending up_testset() implementation from the arch/ - directories and into libs/libc/machine. - - That assumes, of course, that the spinlock operation can - be perform with only user level priveleges. That may not - always be the case. - - For these reasons, the selection is marked EXPERIMENTAL. - Status: Open - Priority: Low. These are new interfaces and not yet in wide use. The - use case for these interfaces is not well defined, but I - think it would be restricted to multi-core CPU implmentations - which are not widespread in the embedded world. - o Message Queues (sched/mqueue) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/arch/Kconfig b/arch/Kconfig index efa78f5a16..77db3d3fec 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -66,6 +66,7 @@ config ARCH_SIM select ARCH_HAVE_TLS select ARCH_HAVE_TICKLESS select ARCH_HAVE_POWEROFF + select ARCH_HAVE_TESTSET select SERIAL_CONSOLE ---help--- Linux/Cywgin user-mode simulation. @@ -79,6 +80,7 @@ config ARCH_XTENSA bool "Xtensa" select ARCH_HAVE_STACKCHECK select ARCH_HAVE_CUSTOMOPT + select ARCH_HAVE_TESTSET ---help--- Cadence® Tensilica® Xtensa® actictures. @@ -221,6 +223,10 @@ config ARCH_HAVE_RESET bool default n +config ARCH_HAVE_TESTSET + bool + default n + config ARCH_HAVE_FETCHADD bool default n diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 8b8d3af60c..1b30cdb22b 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -495,6 +495,7 @@ config ARCH_CORTEXM3 select ARCH_HAVE_LAZYFPU select ARCH_HAVE_HIPRI_INTERRUPT select ARCH_HAVE_RESET + select ARCH_HAVE_TESTSET select ARCH_HAVE_HARDFAULT_DEBUG select ARCH_HAVE_MEMFAULT_DEBUG @@ -511,6 +512,7 @@ config ARCH_CORTEXM4 select ARCH_HAVE_LAZYFPU select ARCH_HAVE_HIPRI_INTERRUPT select ARCH_HAVE_RESET + select ARCH_HAVE_TESTSET select ARCH_HAVE_HARDFAULT_DEBUG select ARCH_HAVE_MEMFAULT_DEBUG @@ -524,6 +526,7 @@ config ARCH_CORTEXM7 select ARCH_HAVE_LAZYFPU select ARCH_HAVE_HIPRI_INTERRUPT select ARCH_HAVE_RESET + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE select ARCH_HAVE_HARDFAULT_DEBUG select ARCH_HAVE_MEMFAULT_DEBUG @@ -533,6 +536,7 @@ config ARCH_CORTEXA5 default n select ARCH_HAVE_MMU select ARCH_USE_MMU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXA8 @@ -540,6 +544,7 @@ config ARCH_CORTEXA8 default n select ARCH_HAVE_MMU select ARCH_USE_MMU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXA9 @@ -547,12 +552,14 @@ config ARCH_CORTEXA9 default n select ARCH_HAVE_MMU select ARCH_USE_MMU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR4 bool default n select ARCH_HAVE_MPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR4F @@ -560,12 +567,14 @@ config ARCH_CORTEXR4F default n select ARCH_HAVE_MPU select ARCH_HAVE_FPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR5 bool default n select ARCH_HAVE_MPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR5F @@ -573,12 +582,14 @@ config ARCH_CORTEXR5F default n select ARCH_HAVE_MPU select ARCH_HAVE_FPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR7 bool default n select ARCH_HAVE_MPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_CORTEXR7F @@ -586,6 +597,7 @@ config ARCH_CORTEXR7F default n select ARCH_HAVE_MPU select ARCH_HAVE_FPU + select ARCH_HAVE_TESTSET select ARCH_HAVE_COHERENT_DCACHE if ELF || MODULE config ARCH_FAMILY diff --git a/configs/Kconfig b/configs/Kconfig index eded647828..31499240fe 100644 --- a/configs/Kconfig +++ b/configs/Kconfig @@ -2596,6 +2596,13 @@ config BOARDCTL_USBDEVCTRL ---help--- Enables support for the BOARDIOC_USBDEV_CONTROL boardctl() command. +config BOARDCTL_TESTSET + bool "Architecture-specific test/set operation" + default n + ---help--- + Enables support for the BOARDIOC_SPINLOCK boardctl() command. + Architecture specific logic must provide up_testset() interface. + config BOARDCTL_IOCTL bool "Board-specific boardctl() commands" default n diff --git a/configs/boardctl.c b/configs/boardctl.c index 6f5a9a3cec..d8229c29c5 100644 --- a/configs/boardctl.c +++ b/configs/boardctl.c @@ -1,7 +1,7 @@ /**************************************************************************** * configs/boardctl.c * - * Copyright (C) 2015-2017, 2018 Gregory Nutt. All rights reserved. + * Copyright (C) 2015-2017, 2018-2019 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -60,6 +60,10 @@ # include #endif +#ifdef CONFIG_BOARDCTL_TESTSET +# include +#endif + #ifdef CONFIG_LIB_BOARDCTL /**************************************************************************** @@ -420,6 +424,32 @@ int boardctl(unsigned int cmd, uintptr_t arg) break; #endif +#ifdef CONFIG_BOARDCTL_TESTSET + /* CMD: BOARDIOC_TESTSET + * DESCRIPTION: Access architecture-specific up_testset() operation + * ARG: A pointer to a write-able spinlock object. On success + * the preceding spinlock state is returned: 0=unlocked, + * 1=locked. + * CONFIGURATION: CONFIG_BOARDCTL_TESTSET + * DEPENDENCIES: Architecture-specific logic provides up_testset() + */ + + case BOARDIOC_TESTSET: + { + volatile FAR spinlock_t *lock = (volatile FAR spinlock_t *)arg; + + if (lock == NULL) + { + ret = -EINVAL; + } + else + { + ret = up_testset(lock) == SP_LOCKED ? 1 : 0; + } + } + break; +#endif + default: { #ifdef CONFIG_BOARDCTL_IOCTL diff --git a/include/nuttx/arch.h b/include/nuttx/arch.h index 772fcbe639..c0c290c805 100644 --- a/include/nuttx/arch.h +++ b/include/nuttx/arch.h @@ -1,7 +1,7 @@ /**************************************************************************** * include/nuttx/arch.h * - * Copyright (C) 2007-2018 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2019 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without diff --git a/include/nuttx/spinlock.h b/include/nuttx/spinlock.h index 2e5beb3745..83ee79eec6 100644 --- a/include/nuttx/spinlock.h +++ b/include/nuttx/spinlock.h @@ -1,7 +1,7 @@ /**************************************************************************** * include/nuttx/spinlock.h * - * Copyright (C) 2016 Gregory Nutt. All rights reserved. + * Copyright (C) 2016, 2019 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without diff --git a/include/sys/boardctl.h b/include/sys/boardctl.h index 14bc6f3b2f..ee3477494f 100644 --- a/include/sys/boardctl.h +++ b/include/sys/boardctl.h @@ -1,7 +1,7 @@ /**************************************************************************** * include/sys/boardctl.h * - * Copyright (C) 2015-2018 Gregory Nutt. All rights reserved. + * Copyright (C) 2015-2019 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -110,10 +110,18 @@ * DEPENDENCIES: Board logic must provide board__initialize() * * CMD: BOARDIOC_NX_START - * DESCRIPTION: Start the NX servier + * DESCRIPTION: Start the NX server * ARG: None * CONFIGURATION: CONFIG_NX * DEPENDENCIES: Base graphics logic provides nxmu_start() + * + * CMD: BOARDIOC_TESTSET + * DESCRIPTION: Access architecture-specific up_testset() operation + * ARG: A pointer to a write-able spinlock object. On success + * the preceding spinlock state is returned: 0=unlocked, + * 1=locked. + * CONFIGURATION: CONFIG_BOARDCTL_TESTSET + * DEPENDENCIES: Architecture-specific logic provides up_testset() */ #define BOARDIOC_INIT _BOARDIOC(0x0001) @@ -125,6 +133,7 @@ #define BOARDIOC_OS_SYMTAB _BOARDIOC(0x0007) #define BOARDIOC_USBDEV_CONTROL _BOARDIOC(0x0008) #define BOARDIOC_NX_START _BOARDIOC(0x0009) +#define BOARDIOC_TESTSET _BOARDIOC(0x000a) /* If CONFIG_BOARDCTL_IOCTL=y, then board-specific commands will be support. * In this case, all commands not recognized by boardctl() will be forwarded @@ -133,7 +142,7 @@ * User defined board commands may begin with this value: */ -#define BOARDIOC_USER _BOARDIOC(0x0009) +#define BOARDIOC_USER _BOARDIOC(0x000b) /**************************************************************************** * Public Type Definitions diff --git a/libs/libc/pthread/Kconfig b/libs/libc/pthread/Kconfig index e20b25ebed..868e1ab423 100644 --- a/libs/libc/pthread/Kconfig +++ b/libs/libc/pthread/Kconfig @@ -4,23 +4,14 @@ # menu "pthread support" - depends on !CONFIG_DISABLE_PTHREAD + depends on !DISABLE_PTHREAD config PTHREAD_SPINLOCKS bool "pthread spinlock support" default n - depends on SPINLOCK && BUILD_FLAT && EXPERIMENTAL + depends on SPINLOCK && LIB_BOARDCTL + select BOARDCTL_TESTSET ---help--- - Enable EXPERIMENTAL support for pthread spinlocks. - - This is marked EXPERIMENTAL for two reasons (1) the use case is not - fully verified, and (2) it depends on architecture specific - support provided by each architecture that may not be fully - available to the pthread library. - - It also currently depends on CONFIG_BUILD_FLAT because the - critical test and set function (up_testset()) as prototyped in - include/nuttx/spinlock() does not permit an inline function or - macro to be used in user-mode application space. + Enable support for pthread spinlocks. endmenu # pthread support diff --git a/libs/libc/pthread/pthread_spinlock.c b/libs/libc/pthread/pthread_spinlock.c index c7447f48eb..ef188ef50b 100644 --- a/libs/libc/pthread/pthread_spinlock.c +++ b/libs/libc/pthread/pthread_spinlock.c @@ -40,20 +40,19 @@ #include #include +#include + #include #include /* The architecture specific spinlock.h header file must provide the * following: * - * SP_LOCKED - A definition of the locked state value (usually 1) - * SP_UNLOCKED - A definition of the unlocked state value (usually 0) - * spinlock_t - The type of a spinlock memory object (usually uint8_t). - * SP_DSB(), - Memory barriers - * SP_DMB + * SP_LOCKED - A definition of the locked state value (usually 1) + * SP_UNLOCKED - A definition of the unlocked state value (usually 0) + * spinlock_t - The type of a spinlock memory object (usually uint8_t). */ -#include #include #ifdef CONFIG_PTHREAD_SPINLOCKS @@ -184,6 +183,7 @@ int pthread_spin_destroy(pthread_spinlock_t *lock) int pthread_spin_lock(pthread_spinlock_t *lock) { pthread_t me = pthread_self(); + int ret; DEBUGASSERT(lock != NULL); if (lock == NULL) @@ -195,14 +195,32 @@ int pthread_spin_lock(pthread_spinlock_t *lock) return EDEADLOCK; } - while (up_testset(&lock->sp_lock) == SP_LOCKED) + /* Loop until we successfully take the spinlock (i.e., until the previous + * state of the spinlock was SP_UNLOCKED). NOTE that the test/set operaion + * is performed via boardctl() to avoid a variety of issues. + */ + + do { - SP_DSB(); + ret = boardctl(BOARDIOC_TESTSET, (uintptr_t)&lock->sp_lock); + } + while (ret == 1); + + /* Check for success (previous state was SP_UNLOCKED) */ + + if (ret == 0) + { + lock->sp_holder = me; + } + else + { + /* An error of some kind is the only other possibility */ + + DEBUGASSERT(ret < 0); + ret = -ret; } - lock->sp_holder = me; - SP_DMB(); - return OK; + return ret; } /**************************************************************************** @@ -229,26 +247,40 @@ int pthread_spin_lock(pthread_spinlock_t *lock) int pthread_spin_trylock(pthread_spinlock_t *lock) { pthread_t me = pthread_self(); + int ret; DEBUGASSERT(lock != NULL); if (lock == NULL) { - return EINVAL; + ret = EINVAL; } else if (lock->sp_holder == me) { - return OK; - } - else if (up_testset(&lock->sp_lock) == SP_LOCKED) - { - lock->sp_holder = me; - SP_DMB(); - return OK; + ret = OK; } else { - return EBUSY; + /* Perform the test/set operation via boardctl() */ + + ret = boardctl(BOARDIOC_TESTSET, (uintptr_t)&lock->sp_lock); + switch (ret) + { + case 0: /* Previously unlocked. We hold the spinlock */ + lock->sp_holder = me; + break; + + case 1: /* Previously locked. We did not get the spinlock */ + ret = EBUSY; + break; + + default: + DEBUGASSERT(ret < 0); + ret = -ret; + break; + } } + + return ret; } /**************************************************************************** diff --git a/sched/Kconfig b/sched/Kconfig index 9fb6fe9dbd..ba04275d16 100644 --- a/sched/Kconfig +++ b/sched/Kconfig @@ -257,6 +257,7 @@ menu "Tasks and Scheduling" config SPINLOCK bool "Support Spinlocks" default n + depends on ARCH_HAVE_TESTSET ---help--- Enables suppport for spinlocks. Spinlocks are current used only for SMP suppport. diff --git a/sched/semaphore/spinlock.c b/sched/semaphore/spinlock.c index 6d58dfcc3c..79782f4f2d 100644 --- a/sched/semaphore/spinlock.c +++ b/sched/semaphore/spinlock.c @@ -458,6 +458,7 @@ void spin_unlockr(FAR struct spinlock_s *lock) * ****************************************************************************/ +#ifdef CONFIG_SMP void spin_setbit(FAR volatile cpu_set_t *set, unsigned int cpu, FAR volatile spinlock_t *setlock, FAR volatile spinlock_t *orlock) @@ -499,6 +500,7 @@ void spin_setbit(FAR volatile cpu_set_t *set, unsigned int cpu, spin_unlock(setlock); up_irq_restore(flags); } +#endif /**************************************************************************** * Name: spin_clrbit @@ -517,6 +519,7 @@ void spin_setbit(FAR volatile cpu_set_t *set, unsigned int cpu, * ****************************************************************************/ +#ifdef CONFIG_SMP void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu, FAR volatile spinlock_t *setlock, FAR volatile spinlock_t *orlock) @@ -560,5 +563,6 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu, spin_unlock(setlock); up_irq_restore(flags); } +#endif #endif /* CONFIG_SPINLOCK */