nuttx/nuttx-update
1
2
Fork 1
Code Issues 1 Pull requests 1 Projects Releases Packages Wiki Activity Actions

spinlock: remove recursive locks with write_lock_irqsave/read_lock_irqsave

Browse source 
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 <hujun5@xiaomi.com>
This commit is contained in:
hujun5 2024-11-26 07:46:31 +08:00 committed by Xiang Xiao
parent 33226a9bb7
commit f5136b2afa
2 changed files with 21 additions and 123 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
include/nuttx/spinlock.h
View file

@ -995,25 +995,19 @@ static inline_function void write_unlock(FAR volatile rwlock_t *lock)
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), disable local * The argument lock should be specified,
* interrupts and take the global read write spinlock (g_irq_rw_spin)
* and increase g_irq_rw_spin.
*
* If the argument lock is specified,
* disable local interrupts and take the lock spinlock and return * disable local interrupts and take the lock spinlock and return
* the interrupt state. * the interrupt state.
* *
* NOTE: This API is very simple to protect data (e.g. H/W register * 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) * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
* *
* If SMP is not enabled: * If SMP is not enabled:
* This function is equivalent to up_irq_save(). * This function is equivalent to up_irq_save().
* *
* Input Parameters: * Input Parameters:
* lock - Caller specific spinlock. If specified NULL, g_irq_spin is used * lock - Caller specific spinlock, not NULL.
* and can be nested. Otherwise, nested call for the same lock
* would cause a deadlock
* *
* Returned Value: * Returned Value:
* An opaque, architecture-specific value that represents the state of * An opaque, architecture-specific value that represents the state of
@ -1032,11 +1026,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock);
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified, release the lock and
* 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
* restore the interrupt state as it was prior to the previous call to * restore the interrupt state as it was prior to the previous call to
* read_lock_irqsave(lock). * 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(). * This function is equivalent to up_irq_restore().
* *
* Input Parameters: * 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 * flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to read_lock_irqsave(lock); * 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: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified,
* 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,
* disable local interrupts and take the lock spinlock and return * disable local interrupts and take the lock spinlock and return
* the interrupt state. * 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(). * This function is equivalent to up_irq_save().
* *
* Input Parameters: * Input Parameters:
* lock - Caller specific spinlock. If specified NULL, g_irq_spin is used * lock - Caller specific spinlock, not NULL.
* and can be nested. Otherwise, nested call for the same lock
* would cause a deadlock
* *
* Returned Value: * Returned Value:
* An opaque, architecture-specific value that represents the state of * An opaque, architecture-specific value that represents the state of
@ -1104,13 +1086,7 @@ irqstate_t write_lock_irqsave(FAR rwlock_t *lock);
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified, release the lock and
* 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
* restore the interrupt state as it was prior to the previous call to * restore the interrupt state as it was prior to the previous call to
* write_lock_irqsave(lock). * 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(). * This function is equivalent to up_irq_restore().
* *
* Input Parameters: * 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 * flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to write_lock_irqsave(lock); * the interrupts prior to the call to write_lock_irqsave(lock);

94
sched/irq/irq_spinlock.c
View file

@ -47,17 +47,6 @@ volatile spinlock_t g_irq_spin = SP_UNLOCKED;
volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS]; 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 * Public Functions
****************************************************************************/ ****************************************************************************/
@ -69,11 +58,7 @@ static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS];
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the 'lock' argument is not specified (i.e. NULL), disable local * The argument lock should be specified,
* interrupts and take the global read write spinlock (g_irq_rwspin)
* and increase g_irq_rwspin.
*
* If the 'lock' argument is specified,
* disable local interrupts and take the lock spinlock and return * disable local interrupts and take the lock spinlock and return
* the interrupt state. * 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(). * This function is equivalent to up_irq_save().
* *
* Input Parameters: * Input Parameters:
* lock - Caller specific spinlock. If specified NULL, g_irq_spin is used * lock - Caller specific spinlock, not NULL.
* and can be nested. Otherwise, nested call for the same lock
* would cause a deadlock
* *
* Returned Value: * Returned Value:
* An opaque, architecture-specific value that represents the state of * 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; irqstate_t ret;
ret = up_irq_save(); ret = up_irq_save();
if (NULL == lock)
{
read_lock(&g_irq_rwspin);
}
else
{
read_lock(lock); read_lock(lock);
}
return ret; return ret;
} }
@ -117,11 +93,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified, release the lock and
* 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
* restore the interrupt state as it was prior to the previous call to * restore the interrupt state as it was prior to the previous call to
* read_lock_irqsave(lock). * 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(). * This function is equivalent to up_irq_restore().
* *
* Input Parameters: * 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 * flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to read_lock_irqsave(lock); * 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) 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); up_irq_restore(flags);
} }
@ -158,13 +122,7 @@ void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified,
* 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,
* disable local interrupts and take the lock spinlock and return * disable local interrupts and take the lock spinlock and return
* the interrupt state. * 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(). * This function is equivalent to up_irq_save().
* *
* Input Parameters: * Input Parameters:
* lock - Caller specific spinlock. If specified NULL, g_irq_spin is used * lock - Caller specific spinlock, not NULL.
* and can be nested. Otherwise, nested call for the same lock
* would cause a deadlock
* *
* Returned Value: * Returned Value:
* An opaque, architecture-specific value that represents the state of * 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; irqstate_t ret;
ret = up_irq_save(); 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; return ret;
} }
@ -215,13 +157,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock)
* *
* Description: * Description:
* If SMP is enabled: * If SMP is enabled:
* If the argument lock is not specified (i.e. NULL), * The argument lock should be specified, release the lock and
* 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
* restore the interrupt state as it was prior to the previous call to * restore the interrupt state as it was prior to the previous call to
* write_lock_irqsave(lock). * write_lock_irqsave(lock).
* *
@ -229,7 +165,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock)
* This function is equivalent to up_irq_restore(). * This function is equivalent to up_irq_restore().
* *
* Input Parameters: * 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 * flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to write_lock_irqsave(lock); * 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) 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); up_irq_restore(flags);
} }