This is an automated email from the ASF dual-hosted git repository.
xiaoxiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new f5136b2afa spinlock: remove recursive locks with
write_lock_irqsave/read_lock_irqsave
f5136b2afa is described below
commit f5136b2afa683946a1a55024fc6fbb168c4c5cf7
Author: hujun5 <[email protected]>
AuthorDate: Tue Nov 26 07:46:31 2024 +0800
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 <[email protected]>
---
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);
}
