From f5136b2afa683946a1a55024fc6fbb168c4c5cf7 Mon Sep 17 00:00:00 2001 From: hujun5 Date: Tue, 26 Nov 2024 07:46:31 +0800 Subject: [PATCH] spinlock: remove recursive locks with write_lock_irqsave/read_lock_irqsave reason: 1 There is a similar PR, https://github.com/apache/nuttx/pull/14079, 2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave. 3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance 4 Nested spinlock is also not available in Linux. 5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed. Signed-off-by: hujun5 --- include/nuttx/spinlock.h | 42 ++++------------ sched/irq/irq_spinlock.c | 102 +++++---------------------------------- 2 files changed, 21 insertions(+), 123 deletions(-) diff --git a/include/nuttx/spinlock.h b/include/nuttx/spinlock.h index c6a0cd43a8..31d4becd08 100644 --- a/include/nuttx/spinlock.h +++ b/include/nuttx/spinlock.h @@ -995,25 +995,19 @@ static inline_function void write_unlock(FAR volatile rwlock_t *lock) * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), disable local - * interrupts and take the global read write spinlock (g_irq_rw_spin) - * and increase g_irq_rw_spin. - * - * If the argument lock is specified, + * The argument lock should be specified, * disable local interrupts and take the lock spinlock and return * the interrupt state. * * NOTE: This API is very simple to protect data (e.g. H/W register - * or internal data structure) in SMP mode. But do not use this API + * or internal data structure) in SMP mode. Do not use this API * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait) * * If SMP is not enabled: * This function is equivalent to up_irq_save(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used - * and can be nested. Otherwise, nested call for the same lock - * would cause a deadlock + * lock - Caller specific spinlock, not NULL. * * Returned Value: * An opaque, architecture-specific value that represents the state of @@ -1032,11 +1026,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock); * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * decrement the call counter (g_irq_rw_spin) and restore the interrupt - * state as it was prior to the previous call to read_lock_irqsave(NULL). - * - * If the argument lock is specified, release the lock and + * The argument lock should be specified, release the lock and * restore the interrupt state as it was prior to the previous call to * read_lock_irqsave(lock). * @@ -1044,7 +1034,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock); * This function is equivalent to up_irq_restore(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used. + * lock - Caller specific spinlock, not NULL. * * flags - The architecture-specific value that represents the state of * the interrupts prior to the call to read_lock_irqsave(lock); @@ -1065,13 +1055,7 @@ void read_unlock_irqrestore(FAR rwlock_t *lock, irqstate_t flags); * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * disable local interrupts and take the global spinlock (g_irq_rw_spin) - * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then - * the counter on the CPU is incremented to allow nested calls and return - * the interrupt state. - * - * If the argument lock is specified, + * The argument lock should be specified, * disable local interrupts and take the lock spinlock and return * the interrupt state. * @@ -1083,9 +1067,7 @@ void read_unlock_irqrestore(FAR rwlock_t *lock, irqstate_t flags); * This function is equivalent to up_irq_save(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used - * and can be nested. Otherwise, nested call for the same lock - * would cause a deadlock + * lock - Caller specific spinlock, not NULL. * * Returned Value: * An opaque, architecture-specific value that represents the state of @@ -1104,13 +1086,7 @@ irqstate_t write_lock_irqsave(FAR rwlock_t *lock); * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * decrement the call counter (g_irq_rw_spin_count[cpu]) and if it - * decrements to zero then release the spinlock (g_irq_rw_spin) and - * restore the interrupt state as it was prior to the previous call to - * write_lock_irqsave(NULL). - * - * If the argument lock is specified, release the lock and + * The argument lock should be specified, release the lock and * restore the interrupt state as it was prior to the previous call to * write_lock_irqsave(lock). * @@ -1118,7 +1094,7 @@ irqstate_t write_lock_irqsave(FAR rwlock_t *lock); * This function is equivalent to up_irq_restore(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used. + * lock - Caller specific spinlock, not NULL. * * flags - The architecture-specific value that represents the state of * the interrupts prior to the call to write_lock_irqsave(lock); diff --git a/sched/irq/irq_spinlock.c b/sched/irq/irq_spinlock.c index 5b20e77a5a..3c94cbb405 100644 --- a/sched/irq/irq_spinlock.c +++ b/sched/irq/irq_spinlock.c @@ -47,17 +47,6 @@ volatile spinlock_t g_irq_spin = SP_UNLOCKED; volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS]; -#ifdef CONFIG_RW_SPINLOCK -/* Used for access control */ - -static volatile rwlock_t g_irq_rwspin = RW_SP_UNLOCKED; - -/* Handles nested calls to write_lock_irqsave and write_unlock_irqrestore */ - -static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS]; - -#endif - /**************************************************************************** * Public Functions ****************************************************************************/ @@ -69,11 +58,7 @@ static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS]; * * Description: * If SMP is enabled: - * If the 'lock' argument is not specified (i.e. NULL), disable local - * interrupts and take the global read write spinlock (g_irq_rwspin) - * and increase g_irq_rwspin. - * - * If the 'lock' argument is specified, + * The argument lock should be specified, * disable local interrupts and take the lock spinlock and return * the interrupt state. * @@ -85,9 +70,7 @@ static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS]; * This function is equivalent to up_irq_save(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used - * and can be nested. Otherwise, nested call for the same lock - * would cause a deadlock + * lock - Caller specific spinlock, not NULL. * * Returned Value: * An opaque, architecture-specific value that represents the state of @@ -100,14 +83,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock) irqstate_t ret; ret = up_irq_save(); - if (NULL == lock) - { - read_lock(&g_irq_rwspin); - } - else - { - read_lock(lock); - } + read_lock(lock); return ret; } @@ -117,11 +93,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock) * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * decrement the call counter (g_irq_rwspin) and restore the interrupt - * state as it was prior to the previous call to read_lock_irqsave(NULL). - * - * If the argument lock is specified, release the lock and + * The argument lock should be specified, release the lock and * restore the interrupt state as it was prior to the previous call to * read_lock_irqsave(lock). * @@ -129,7 +101,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock) * This function is equivalent to up_irq_restore(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used. + * lock - Caller specific spinlock, not NULL. * * flags - The architecture-specific value that represents the state of * the interrupts prior to the call to read_lock_irqsave(lock); @@ -141,15 +113,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock) void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags) { - if (NULL == lock) - { - read_unlock(&g_irq_rwspin); - } - else - { - read_unlock(lock); - } - + read_unlock(lock); up_irq_restore(flags); } @@ -158,13 +122,7 @@ void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags) * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * disable local interrupts and take the global spinlock (g_irq_rwspin) - * if the call counter (g_irq_rwspin_count[cpu]) equals to 0. Then - * the counter on the CPU is incremented to allow nested calls and return - * the interrupt state. - * - * If the argument lock is specified, + * The argument lock should be specified, * disable local interrupts and take the lock spinlock and return * the interrupt state. * @@ -176,9 +134,7 @@ void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags) * This function is equivalent to up_irq_save(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used - * and can be nested. Otherwise, nested call for the same lock - * would cause a deadlock + * lock - Caller specific spinlock, not NULL. * * Returned Value: * An opaque, architecture-specific value that represents the state of @@ -191,21 +147,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock) irqstate_t ret; ret = up_irq_save(); - if (NULL == lock) - { - int me = this_cpu(); - if (0 == g_irq_rwspin_count[me]) - { - write_lock(&g_irq_rwspin); - } - - g_irq_rwspin_count[me]++; - DEBUGASSERT(0 != g_irq_rwspin_count[me]); - } - else - { - write_lock(lock); - } + write_lock(lock); return ret; } @@ -215,13 +157,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock) * * Description: * If SMP is enabled: - * If the argument lock is not specified (i.e. NULL), - * decrement the call counter (g_irq_rwspin_count[cpu]) and if it - * decrements to zero then release the spinlock (g_irq_rwspin) and - * restore the interrupt state as it was prior to the previous call to - * write_lock_irqsave(NULL). - * - * If the argument lock is specified, release the lock and + * The argument lock should be specified, release the lock and * restore the interrupt state as it was prior to the previous call to * write_lock_irqsave(lock). * @@ -229,7 +165,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock) * This function is equivalent to up_irq_restore(). * * Input Parameters: - * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used. + * lock - Caller specific spinlock, not NULL. * * flags - The architecture-specific value that represents the state of * the interrupts prior to the call to write_lock_irqsave(lock); @@ -241,21 +177,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock) void write_unlock_irqrestore(rwlock_t *lock, irqstate_t flags) { - if (NULL == lock) - { - int me = this_cpu(); - DEBUGASSERT(0 < g_irq_rwspin_count[me]); - g_irq_rwspin_count[me]--; - - if (0 == g_irq_rwspin_count[me]) - { - write_unlock(&g_irq_rwspin); - } - } - else - { - write_unlock(lock); - } + write_unlock(lock); up_irq_restore(flags); }